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

add country/pollutant CLI options to airbase all #40

Merged
merged 10 commits into from
Dec 7, 2022

Conversation

avaldebe
Copy link
Collaborator

At work we need to download all observations/sites for 6 different pollutants.
At the moment we invoke the CLI 6 times as follows:

airbase pollutant SO2
airbase pollutant PM10
airbase pollutant O3
airbase pollutant NO2
airbase pollutant CO
airbase pollutant NO
airbase pollutant PM2.5

after merging this PR we'll be able to write

airbase all -p SO2 -p PM10 -p O3 -p NO2 -p CO -p NO -p PM2.5

It will also be possible to restrict the download to specific countries on the same CLI sub-command, e.g. downloading the same pollutants only for Norwegian, Danish and Finish sites

airbase all -p SO2 -p PM10 -p O3 -p NO2 -p CO -p NO -p PM2.5 -c NO -c DK -c FI

It might be possible to consolidate the other sub-commands (after a deprecation cycle), as their functionality is provided here.

@avaldebe avaldebe self-assigned this Oct 27, 2022
@avaldebe avaldebe added the enhancement New feature or request label Oct 27, 2022
@JohnPaton
Copy link
Owner

Ah nice, so the arguments act like filters on all? That makes sense to me from a user perspective. This is probably faster with the parallelisation as well.

@avaldebe
Copy link
Collaborator Author

Ah nice, so the arguments act like filters on all?

Yes, it restricts/focuses the download to only a few pollutant/countries

That makes sense to me from a user perspective. This is probably faster with the parallelisation as well.

It should be faster to request multiple pollutant/countries at once than requesting them one by one, but I would guess that the difference is small.

This is mostly an UI/UX update based on the feedback from my colleague putting the CLI in production.

What do you think about consolidating the CLI sub-commands?
airbase pollutant NO2 --country NO and airbase country NO --pollutant NO2 are
equivalent to airbase all --pollutant NO2 --country NO, so we could remove the sub-commands altogether and write airbase --pollutant NO2 --country NO (implicit all sub-command)
I can write deprecating workings for airbase pollutant and airbase country
and remove them from v1.0 on a separate PR

@JohnPaton
Copy link
Owner

Yes I think this sounds like a good idea. Let's indeed deprecate them for now and add the new API already, and then we can remove in 1.0

@avaldebe
Copy link
Collaborator Author

avaldebe commented Nov 3, 2022

I added added a new CLI sub-command airbase download, with the same options and functionality of airbase all. Now airbase all, airbase country and airbase pollutant write a deprecation message before starting the download.

I would like to add a short entry to the README and the docs about airbase download, and maybe some tests before asking for a review.

@avaldebe avaldebe marked this pull request as ready for review November 16, 2022 15:29
@avaldebe
Copy link
Collaborator Author

I added the CLI to the README. The docs need some updating after merging #41

@avaldebe avaldebe requested a review from JohnPaton November 17, 2022 10:41
Copy link
Owner

@JohnPaton JohnPaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nice, thanks for updating the readme as well! Just a few quick questions but I think this looks pretty much good to go.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
airbase/airbase.py Show resolved Hide resolved
avaldebe and others added 2 commits December 7, 2022 08:58
Co-authored-by: John Paton <john@johnpaton.net>
Copy link
Owner

@JohnPaton JohnPaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, as always!

@avaldebe avaldebe merged commit 7b3f9ca into JohnPaton:master Dec 7, 2022
@avaldebe avaldebe deleted the more-cli-options branch December 7, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants