-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Made OSM header accessible #24
Made OSM header accessible #24
Conversation
decode.go
Outdated
@@ -15,6 +15,7 @@ import ( | |||
|
|||
"github.com/golang/protobuf/proto" | |||
"github.com/qedus/osmpbf/OSMPBF" | |||
"sync" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May you please move this import up?
decode.go
Outdated
func (dec *Decoder) Header() (*Header, error) { | ||
// synchronize with possible simultaneous call to 'Start()' | ||
dec.mu.Lock() | ||
defer dec.mu.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can sync.Once
be a better choice here?
@karsten42 thanks for this! Have you thought about just having the |
Yes, I thought about that. |
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 |
@karsten42 and @AlekSi both good points! I concede. |
@AlekSi I made your suggested changes. Good remark using sync.Once. I also fixed the tests I broke (shame on me). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some minor issues, but I will fix them myself in the next PR. Thank you!
@jongillham PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. All my comments are suggestions that don't necessarily need to be addressed for the code to be merged.
decode.go
Outdated
@@ -33,6 +34,24 @@ var ( | |||
} | |||
) | |||
|
|||
type Bbox struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better as BBox
which better matches the osmformat.proto
file of HeaderBBox
or maybe even BoundingBox
which is more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
decode.go
Outdated
// Get the file header | ||
func (dec *Decoder) Header() (*Header, error) { | ||
// deserialize the file header | ||
err := dec.readOSMHeader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
decode.go
Outdated
@@ -157,6 +184,8 @@ func (dec *Decoder) Start(n int) error { | |||
// start reading OSMData | |||
go func() { | |||
var inputIndex int | |||
var blobHeader *OSMPBF.BlobHeader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@jongillham All very good remarks! I adapted the code accordingly. |
decode.go
Outdated
func (dec *Decoder) readOSMHeader() error { | ||
var err error | ||
dec.headerOnce.Do(func() { | ||
blobHeader, blob, err := dec.readFileBlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the err
here will shadow the outer err
making readOSMHeader()
always return nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.