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

chore: compression level as only option #162

Merged
merged 6 commits into from
Oct 11, 2024

Conversation

uriii3
Copy link
Collaborator

@uriii3 uriii3 commented Oct 10, 2024

Until now, there existed 2 options: --netcdf-compression-enabled and --netcdf-compression-level.

The enabled was a boolean to allow for compression, the level was an integer between 0 and 9 to allow to choose the level of compression.

We squashed the two options together, making users choose a level of compression (maybe would be better to recommend to use 1 first) and that is it. The default option would remain 0, as that is the no compression type.

@uriii3 uriii3 requested a review from renaudjester October 10, 2024 14:59
@click.option(
"--netcdf-compression-level",
type=click.IntRange(0, 9),
default=0,
Copy link
Collaborator

@renaudjester renaudjester Oct 10, 2024

Choose a reason for hiding this comment

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

I guess the behavior should be the following:

copernicusmarine subset -i whateve -v somev -> netcdf_compression_level = None or 0 or False
copernicusmarine subset -i whateve -v somev --netcdf-compression-level  -> netcdf_compression_level = 1
copernicusmarine subset -i whateve -v somev --netcdf-compression-level  n -> netcdf_compression_level = n

(from what said on this: https://cms-change.atlassian.net/browse/CMT-122)

See: https://stackoverflow.com/questions/60741970/optional-cli-arguments-with-python-click-library-option/78226412#78226412

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created the change to allow for this, maybe some tests to check it would be nice: I will add them later (after the presentation hehe)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about the python interface? Should it also accept "True" as an option? Would be weird, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No because in python flags don't exist :D so I think it's fine like this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It works well right? You made sure that if you just use the flag with no number it compresses to 1? If yes, looks good to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I made a test for that!

@uriii3 uriii3 requested a review from renaudjester October 11, 2024 15:27
@click.option(
"--netcdf-compression-level",
type=click.IntRange(0, 9),
default=0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No because in python flags don't exist :D so I think it's fine like this!

@click.option(
"--netcdf-compression-level",
type=click.IntRange(0, 9),
default=0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It works well right? You made sure that if you just use the flag with no number it compresses to 1? If yes, looks good to me

@uriii3 uriii3 merged commit 1c6b73a into copernicusmarine-toolbox-v2 Oct 11, 2024
2 checks passed
@uriii3 uriii3 deleted the compression-level branch October 11, 2024 16:15
renaudjester pushed a commit that referenced this pull request Oct 28, 2024
No need to specify `--netcdf-compression-enabled` to be able to compress. Works also as a flag to compress to level 1.
renaudjester pushed a commit that referenced this pull request Oct 28, 2024
No need to specify `--netcdf-compression-enabled` to be able to compress. Works also as a flag to compress to level 1.
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