-
Notifications
You must be signed in to change notification settings - Fork 31
provide info metadata to Nodes, Ways and Relations #40
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
base: master
Are you sure you want to change the base?
Conversation
TeXitoi
left a comment
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.
Thanks for the contribution!
src/objects.rs
Outdated
| #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Hash, Serialize, Deserialize)] | ||
| pub struct Info { | ||
| /// The timestamp when the object was introduced. | ||
| pub timestamp: Option<NaiveDateTime>, |
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'm a bit worried by this:
- there is 2 options inside each ones, that makes it difficult to use
- the is_visible is obviously true if there is no info
- all this option thing will use a non negligible amount of memory.
I don't have yet any solution to all of this.
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.
Mh yeah I'm not happy with the double Option either.
I think it makes sense to keep the Info object, as the osm protobuf definition provides some more fields like changeset and uid, so one might want to extend it in the future.
Not sure wether one could find an Info without a timestamp in the wild. I certainly didn't find one. Maybe it would make sense to make the timestamp non-optional?
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.
Sorry, forgot about this PR.
Yeah, I think we can:
- make timestamp non optional, outputing 0 in the rare case when we have no info
Option<NonZeroI64>, with some appropriate getters for ease of use. A timestamp of 0 is impossible as OSM didn't exist in 1970.
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.
My OSM related project is on ice, so I won't be able to help further with this PR, sorry. Good luck!
This metadate is used in history dumps, as can be found at https://planet.openstreetmap.org/pbf/full-history/
…bf file With cfg='full-metadata' enabled, these fields are available in an 'info' field of Node/Way/Relation object: - version - timestamp, as an i64 number, in milliseconds since 1970 epoch - changeset - user id - user name - visible, set to true by default This is greatly inspired from PR #TeXitoi#40, with the addition of more fields.
Thanks for creating this library, I'm using it for a while now.
I needed this patch to process OSM history dumps, like the ones found at https://planet.openstreetmap.org/pbf/full-history/.
I was able to parse some 400MB large extracts using my patch without a problem.
This adds chrono as a dependency, as I felt that would be the best way to represent the timestamp.