Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: fetch-feeds command #5

wants to merge 7 commits into from

Conversation

Ananya2001-an
Copy link
Collaborator

@Ananya2001-an Ananya2001-an commented Jun 7, 2023

TODO

  • Concurrency as an option in fetch-feeds
  • search option in list-feeds

@Ananya2001-an
Copy link
Collaborator Author

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?

gtfs/__main__.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@nilsnolde
Copy link
Member

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?

@Ananya2001-an
Copy link
Collaborator Author

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?

@nilsnolde
Copy link
Member

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.

@Ananya2001-an
Copy link
Collaborator Author

Ok cool

@Ananya2001-an Ananya2001-an marked this pull request as ready for review June 13, 2023 15:22
gtfs/__main__.py Outdated Show resolved Hide resolved
gtfs/__main__.py Outdated Show resolved Hide resolved
gtfs/__main__.py Outdated Show resolved Hide resolved
gtfs/__main__.py Outdated Show resolved Hide resolved
gtfs/feed_source.py Outdated Show resolved Hide resolved
gtfs/feed_source.py Outdated Show resolved Hide resolved
gtfs/__main__.py Outdated Show resolved Hide resolved
Copy link
Member

@nilsnolde nilsnolde left a 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.

@nilsnolde
Copy link
Member

let me know once you're good with the changes for review, then I'll have a closer look again.

@Ananya2001-an
Copy link
Collaborator Author

Hey @nilsnolde sorry for the late reply.....you can review these changes for now :)

@Ananya2001-an
Copy link
Collaborator Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants