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

Command refactor #15

Merged
merged 3 commits into from
Aug 11, 2021
Merged

Command refactor #15

merged 3 commits into from
Aug 11, 2021

Conversation

GeoWill
Copy link
Contributor

@GeoWill GeoWill commented Jul 3, 2021

I don't think this is 'done', but it is easier to work with. In particular I don't think I've got the control with the 'pretty' flag quite right. However I'm hopeful it can be iterated from here.
I think we can use Rich 'everywhere' if I've understood the documentation correctly. I think a way to manage this is to just output 'logging' from the Scrapers, and have nice cli reporting handled in the Commands. This way when the scraper_worker_handler runs the scraper directly (rather than through the Command) we get the logging with out the boxes/columns/progress bars.

I've merged this into #13 in order to see how it plays with the handlers and the whole sam deploy.

This lets us run a command like:
$ python manage.py councillors -t example -v

Which is useful when trying out commands that run multiple scrapers.
This pulls what was in the councillor command into the
PerCouncilCommandBase class. This is so that it can be reused.

Along the way I've:
- dropped reporting on skipped/missing/etc councils with the progress
bars - these can come back, but weren't working as intended.
- Made the command runner explicitly call `execute` rather than it
appear in the __init__ method.
- Added a 'pretty' flag to manage what output is shown.
@symroe
Copy link
Member

symroe commented Jul 6, 2021

Should we close this PR, as I left comments on the same code in #13?

@GeoWill
Copy link
Contributor Author

GeoWill commented Jul 8, 2021

Should we close this PR, as I left comments on the same code in #13?

I think I'd like to get this merged seperatly - if only to keep #13 smaller. I used it in #13 as much to check/demonstrate the ergonomics as anything else. Any changes that are needed in #13 then won't be as drastic. I'd also like to use the changes in #11. }

@GeoWill GeoWill merged commit 3ddc8f6 into master Aug 11, 2021
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