Closed medwards closed 7 years ago
@bfrog: I could still use a code review on this.
thanks for the ding, will look now
So my only concern is the removal of describing where parse errors occur in the gtfs txt files, which I found useful previously. Some GTFS providers do funny and odd things that aren't a part of the standard. Other than that the work looks great! I'd really appreciate the file/line/error kind be the tuple when relating to errors. If I see that back in I'd gladly merge this.
I apologize, I didn't see that you have since wrapped ParseError in a LineError :+1:
In terms of removing some of the ugliness from ParseError and Error handling in general here saner, perhaps using the error-chain crate would greatly help.
GTFSIterator::from_path is interesting but I don't think its possible to do in Rust without erasing the type specialization of the parse fn and the GTFSIterator itself using an enum to contain all possible iterable types like Calendar, Trip, Stop, etc which really defeats the type safety.
Now that doesn't mean you can't do what you want to do at all, but to do it you would likely need to write a macro!
Ex:
gtfs_iter_for_file!("stops.txt")
I'll leave that to you to have fun working out if you are still interested in writing such a thing
There are quite a few warnings when compiling this as well, just a heads up those should be corrected to the best of your abilities
I'll get to most of this this tonight at the Rust co-working thing here.
This took awhile to rebase so didn't get to my rust fmt run.
Could we please turn off gitcop?
I would suggest lets get this in first then run rust-fmt
gitcop is pretty easy to follow and enforces some relatively simple rules for commit messages that I've already followed for the repo, if its a real downer I can turn it off, otherwise really this could be rebased and the message began with refactor: short desc and gitcop would be happy. I turned it off for now.
Adding the google test data sounds like a great idea. The data in there now is the basic example data from the gtfs site.
LineParseError sounds good
These changes are looking quite nice! Will take a closer look later when I get chance to pull your repo.
Changed LineError to LineParseError
OK at this point things are conceptually complete. I was missing a way to unit test parse_row functions. The last month I worked on adding even more generics so that parse_row functions could accept arbitrary zipped iterators instead of the concrete classes it currently accepts.
This turned out to be extremely painful and instead I created a parse_row_harness test helper that makes the concrete iterator for you from normal vectors. It has to also call parse_row because of lifetime issues that I didn't see an obvious workaround for.
What can I do to help get this merged, or clarify why its not going forward? (ie maybe you're really set on going straight to the csv crate?)
Looks good!
Fixes #2
There's a couple of things I'd like feedback on: I see why the commented out GTFSIterator.from_path doesn't compile, but I still really want something like this. Do you have any ideas? There much ugliness in the Error implementation for ParseError. I'm sure there is a better way to do this but I'm new to Rust.