Add basic read tests of a small pbf file#56
Conversation
jocelynj
force-pushed
the
add-simple-test
branch
from
32eae2e to
e0312e0
Compare
TeXitoi
left a comment
TeXitoi
left a comment
There was a problem hiding this comment.
Good idea! I’m ok with this small file in the repository.
| let mut num_nodes = 0; | ||
| let mut num_ways = 0; | ||
| let mut num_relations = 0; | ||
| let mut sum_ids = 0; |
There was a problem hiding this comment.
Why not doing this sum_id check on the other tests ? If that’s an overflow problem, you can use .wrapping_add(_).
There was a problem hiding this comment.
Mostly because I thought of this check at the end, but we could a similar check on the other tests too. I will add it to all tests.
There was a problem hiding this comment.
sum_node/way/relation_id are now checked on all functions, thanks to the common Stats struct.
I added the check on sum_ids only here, because get_objs_and_deps() is the only function providing both an OsmId and an OsmObj.
|
|
||
| let mut num_nodes = 0; | ||
| let mut num_ways = 0; | ||
| let mut num_relations = 0; |
There was a problem hiding this comment.
I think you can do a struct Stats with these fields (and maybe also sum_ids) and implement FromIterator<OsmObj> for Stats
There was a problem hiding this comment.
Good idea, as it would allow to share the expected values between tests.
jocelynj
force-pushed
the
add-simple-test
branch
from
e0312e0 to
94c063a
Compare
|
I’ve moved all stats to structs, and verified all values with |
TeXitoi
left a comment
TeXitoi
left a comment
There was a problem hiding this comment.
The code can be improved, but I don’t want to annoy you with not really criticals remarks. In the other hand, you might be interested in them. So, we can do it in different ways:
- I merge this as is, and improve the code in another commit, that you can read.
- I make a PR on your PR, allowing you to make a "review" of my modification, to understand them and/or challenge them
- I give you insight to do the" idiomatic" thing, and we exchange till the code is beautiful.
What do you prefer?
| decimicro_lon: (-109.2143679 * 1e7) as i32, | ||
| }, | ||
| Node { | ||
| id: NodeId(1724610082), |
There was a problem hiding this comment.
All these data are new from the last time. To have easily mergeable PR, it is better to not add features during the review, as they might be remarks on them, that will delay the merge of the already merged parts.
There was a problem hiding this comment.
It is on a different commit, so I thought it would be fine. I can remove the second commit if you prefer, and open a second PR afterwards.
|
I'm interested in writing better Rust code, so proposition 3 or 2 would be better for me. But I don't want to take too much of your time. |
|
Is there anything you want me to rewrite in the code? Thanks! |
|
Hi. Sorry, I didn’t had the time to dive into this. I’ll try to advance on this. |
jocelynj
force-pushed
the
add-simple-test
branch
from
139c69c to
76113fd
Compare
jocelynj
force-pushed
the
add-simple-test
branch
2 times, most recently
from
b6fffff to
0a1c69b
Compare
|
Sorry for the delay: I’ve removed the second commit, to focus on merging the simple test addition. Would you have any feedback on the change, or any remark? Thanks! |
|
Hi @TeXitoi , Can this one be approved now? |
|
Thanks, and sorry for the late review. |
3 participants
Hi,
I have added a simple pbf test to make it possible to test adding full metadata (will be pushed on a separate branch).
The new pbf file taken is a 43KB file taken from Geofabrik, and which is the smallest file I could find. If you would prefer to not have pbf files in this git repository, I could try to move it to an external git submodule, and even add bigger files there for more testing - but it would make testing a bit harder.
Thanks!