-
Notifications
You must be signed in to change notification settings - Fork 89
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
Improver api changes #2031
Improver api changes #2031
Conversation
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.
I think this PR is good. All the tests pass and I've just added one question for my interest rather than being a blocker to merging.
cube.rename(self._new_name) | ||
if self._new_units: | ||
cube.convert_units(self._new_units) | ||
if self._coords_to_remove: |
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.
Just curious why you added the underscores to start of these. i.e. why not update it as self.new_name?
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.
Good software design - all data should be hidden within its class so that users don't accidentally (or purposely) change data owned by the class (hidden in the Python sense by convention, not truly 'hidden').
If you are interested, a recommended reading:
"Object-Oriented Design Heuristics" by Arthur J. Riel
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.
Just to feed curiosity, if you wanted to expose this data to users, it would be exposed via setter and getter methods.
Some left over changes to IMPROVER api to support eppgl_precip_type configuration for EPP.
improver.utilities.cube_manipulation
missing from api list (needed for EPP).StandardiseMetadata
plugin so that arguments have been moved to the init to make in line with IMPROVER guidelines.Issues