Made OSM header accessible #24

karsten42 · 2017-06-12T14:12:26Z

As far as I could see there was no way so far to get hand on the OSM header information. It was only being decoded to check if the parser is capable of processing the file.
I added the possibility to get the header via the decoder which reads it from the file and stores it in the struct. The header will only be read one time from the file.

AlekSi · 2017-06-12T14:13:20Z

decode.go

@@ -15,6 +15,7 @@ import (

 	"github.com/golang/protobuf/proto"
 	"github.com/qedus/osmpbf/OSMPBF"
+	"sync"


May you please move this import up?

AlekSi · 2017-06-12T14:14:43Z

decode.go

+func (dec *Decoder) Header() (*Header, error) {
+	// synchronize with possible simultaneous call to 'Start()'
+	dec.mu.Lock()
+	defer dec.mu.Unlock()


Can sync.Once be a better choice here?

jongillham · 2017-06-12T14:30:40Z

@karsten42 thanks for this! Have you thought about just having the Decode() return a *osmpbf.Header, just like it returns *osmpbf.Way etc instead of having a separate Header() function? I understand that only one *osmpbf.Header will be returned but to me if feels more natural with the way the API is right now. What do you think @AlekSi?

karsten42 · 2017-06-12T14:33:37Z

Yes, I thought about that.
But then there is now way of getting the header before you start going through the file, right?
In my application I actually get the header before reading the file to change some parameters.

AlekSi · 2017-06-12T14:35:41Z

Decode reads the next object from the input stream and returns either a pointer to Node, Way or Relation struct representing the underlying OpenStreetMap PBF data, or error encountered.

I think it's too late to break public API after 1.0 even if the signature is not changed. A quick check of https://godoc.org/github.com/qedus/osmpbf?importers showed that people use switch over interface{} with default section, this will change a behavior.

jongillham · 2017-06-12T14:43:16Z

@karsten42 and @AlekSi both good points! I concede.

- refactoring

karsten42 · 2017-06-12T15:03:37Z

@AlekSi I made your suggested changes. Good remark using sync.Once. I also fixed the tests I broke (shame on me).

AlekSi

There are some minor issues, but I will fix them myself in the next PR. Thank you!

AlekSi · 2017-06-12T18:31:48Z

@jongillham PTAL.

jongillham

Nice work. All my comments are suggestions that don't necessarily need to be addressed for the code to be merged.

jongillham · 2017-06-12T22:16:05Z

decode.go

@@ -33,6 +34,24 @@ var (
 	}
 )

+type Bbox struct {


Would this be better as BBox which better matches the osmformat.proto file of HeaderBBox or maybe even BoundingBox which is more descriptive?

I noticed the last commit 51ecde2 changed the name of Header.Bbox to Header.BoundingBox but it didn't include a change of type Bbox struct to type BoundingBox struct.

jongillham · 2017-06-12T22:25:56Z

decode.go

@@ -112,21 +136,24 @@ func (dec *Decoder) SetBufferSize(n int) {
 	dec.buf = bytes.NewBuffer(make([]byte, 0, n))
 }

+// Get the file header
+func (dec *Decoder) Header() (*Header, error) {


I believe this may be simplified to the following without any worries about race conditions:

func (dec *Decoder) Header() (*Header, error) { return dec.header, dec.readOSMHeader() }

The dec.header pointer should always be passed to the calling function after dec.readOSMHeader() has been executed so dec.header will be set unless dec.readOSMHeader() produces an error. The down side of my suggestion is that the race condition, or lack thereof, is less explicit.

jongillham · 2017-06-12T22:36:39Z

decode.go

+// Get the file header
+func (dec *Decoder) Header() (*Header, error) {
+	// deserialize the file header
+	err := dec.readOSMHeader()


Could this be changed to the following in order to keep the err variable scope as small as possible?

if err := dec.readOSMHeader(); err != nil { return nil, err }

jongillham · 2017-06-12T22:39:25Z

decode.go

@@ -157,6 +184,8 @@ func (dec *Decoder) Start(n int) error {
 	// start reading OSMData
 	go func() {
 		var inputIndex int
+		var blobHeader *OSMPBF.BlobHeader


Is there any advantage to putting blobHeader and blob outside the scope of the subsequent for loop? Would they be better declared implicitly at blobHeader, blob, err := dec.readFileBlock()?

- refactoring

karsten42 · 2017-06-16T06:11:56Z

@jongillham All very good remarks! I adapted the code accordingly.

jongillham · 2017-06-21T14:45:14Z

decode.go

+func (dec *Decoder) readOSMHeader() error {
+	var err error
+	dec.headerOnce.Do(func() {
+		blobHeader, blob, err := dec.readFileBlock()


I believe the err here will shadow the outer err making readOSMHeader() always return nil.

You are right, good catch.

…returned

jongillham

Great work! I'm happy with this now. Are you able to squash it down to one commit and then we can merge it if I can't do it automatically with GitHub?

Make OSM header accessible (qedus#24).

Karsten Klompmaker added 2 commits June 12, 2017 08:24

added functionality to retrieve OSM header information from pbf file

8c3be37

added functionality to retrieve OSM header information from pbf file

653d89e

AlekSi requested changes Jun 12, 2017

View reviewed changes

- using sync.Once to synchronize OSM header deserialization

9c2131c

- refactoring

AlekSi self-assigned this Jun 12, 2017

AlekSi approved these changes Jun 12, 2017

View reviewed changes

AlekSi requested a review from jongillham June 12, 2017 18:30

AlekSi assigned jongillham and unassigned AlekSi Jun 12, 2017

jongillham reviewed Jun 12, 2017

View reviewed changes

- renamed OSM header 'Bbox' to 'BoundingBox'

51ecde2

- refactoring

renamed BBox to BoundingBox

624df92

jongillham reviewed Jun 21, 2017

View reviewed changes

fixed error that led to errors in "readOsmHeader" function not being …

dc52637

…returned

jongillham approved these changes Jun 22, 2017

View reviewed changes

jongillham merged commit b4b1bd9 into qedus:master Jun 22, 2017

HellButcher added a commit to HellButcher/osmpbf that referenced this pull request Jun 23, 2017

Merge pull request #1 from qedus/master

1ed960e

Make OSM header accessible (qedus#24).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made OSM header accessible #24

Made OSM header accessible #24

karsten42 commented Jun 12, 2017

AlekSi Jun 12, 2017

AlekSi Jun 12, 2017

jongillham commented Jun 12, 2017

karsten42 commented Jun 12, 2017

AlekSi commented Jun 12, 2017 •

edited

Loading

jongillham commented Jun 12, 2017

karsten42 commented Jun 12, 2017

AlekSi left a comment

AlekSi commented Jun 12, 2017

jongillham left a comment

jongillham Jun 12, 2017

jongillham Jun 19, 2017 •

edited

Loading

jongillham Jun 12, 2017

jongillham Jun 12, 2017

jongillham Jun 12, 2017

karsten42 commented Jun 16, 2017

jongillham Jun 21, 2017

karsten42 Jun 22, 2017

jongillham left a comment

Made OSM header accessible #24

Made OSM header accessible #24

Conversation

karsten42 commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongillham commented Jun 12, 2017

karsten42 commented Jun 12, 2017

AlekSi commented Jun 12, 2017 • edited Loading

jongillham commented Jun 12, 2017

karsten42 commented Jun 12, 2017

AlekSi left a comment

Choose a reason for hiding this comment

AlekSi commented Jun 12, 2017

jongillham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongillham Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karsten42 commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongillham left a comment

Choose a reason for hiding this comment

AlekSi commented Jun 12, 2017 •

edited

Loading

jongillham Jun 19, 2017 •

edited

Loading