-
Notifications
You must be signed in to change notification settings - Fork 9
Removed Indicators bug #55
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
Conversation
ThomasBouche
left a comment
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.
Do we keep the parameters in uppercase since they are no longer constants?
Can you run the pre-commits, because GitHub will returning an error because of this?
you have to install dev extra requires :
pip install meteole[dev]
end then
pre-commit install
pre-commit run --all-files
|
Thank you for your input Thomas Regarding the attribute names, I agree that they should be lowercase and I changed them. I kept the uppercase in the previous commit to not break back-compatibility, as I did not exactly know how you, as the author, wanted to deal with that. We could also keep both versions, an display some sort of warning when the old attribute gets used. If you do want to go this route, I would appreciate some pointers on how to implement this warning and integrate it to your logging framework, as I am not familiar with these. I did add the missing docstrings and ran the pre-commit (seemed fine to me). I added the steps that you mentioned to run them in CONTRIBUTING.md (under ##Continuous integration). I just pushed all the changes to the feature branch. |
ThomasBouche
left a comment
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.
Thanks for the changes and additions to the contributing section !
For the switch to lowercase, the switch is missing in the readme.
I agree with your thoughts on version compatibility. It's a minor breaking change, but it still needs to be managed properly for users.
I'll discuss it with a colleague early next week to get another opinion.
|
Thanks @JacquesOpaux for your contribution ! I will implement warning and notify in changelog |
What?
See #54. Changed attributes INDICATORS and INSTANT_INDICATORS of WeatherForecast to properties to compute them dynamically from the capabilitites attribute.
Have you done?
I changed the attributes to properties, removed them from the different model objects, removed a couple of lines in the tests, changed version in pyptoject.toml to 0.2.3 and updated changelog.
Linked issues: (optional)