-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: fetch-feeds command #5
base: main
Are you sure you want to change the base?
Conversation
694c9cf
to
a3c6417
Compare
Hey @nilsnolde I am facing some issues with the verification part: Actually google's transitfeed package is not maintained anymore and also it's not compatible with python3. They have recommended to use this instead but it's only available as a docker image. There's another package called gtfs-kit which looked good but it's a very big dependency so definitely can't use it.....maybe we have to create our own validator...wdyt? |
IMO we shouldn't validate and focus on providing a good downloader. As you mentioned, there's other software for that. I think it'd be better if we do one functionality at a time, then it'll be easier to review. I suggest to keep the multithreading for now, break out as much logic into functions (not in the main command), so they're unit testable, then get this PR reviewed and focus on the next topic, e.g. date validation. Is that ok for you? |
Yeah that sounds good 👍🏼 so lets keep the concurrency thing for now and is there anything else that concerns you regarding the current code other than the ones you mentioned? |
So far I only skimmed the code. I suggest you bring it into the shape you consider "final", then let me know and I'll give it a more thorough review. |
Ok cool |
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 didn't make it entirely through yet, just a more general comment: all the if/else should be avoided. They can almost all be changed to a single if without else with some re-arranging.
Let's talk in a bit about it.
6de73ca
to
ae2fdec
Compare
let me know once you're good with the changes for review, then I'll have a closer look again. |
Hey @nilsnolde sorry for the late reply.....you can review these changes for now :) |
Hey @nilsnolde I want to continue working on this PR... you can review the changes and tell me if something is needed to be fixed. I would be happy to help :) |
TODO