-
Notifications
You must be signed in to change notification settings - Fork 119
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
Fix a typo in a log message and extend a docstring #235
Fix a typo in a log message and extend a docstring #235
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.
thanks @lumbric for catching this typo and extending the docstrings! some minor comments, then it's good to go.
pyam/core.py
Outdated
scenarios which do not match the criteria and prints a log message or | ||
returns None, if all scenarios match the criteria. | ||
|
||
When called with `exclude_on_fail=True`, the object will be modified. |
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.
Great addition, but let’s be more specific than “modified”: say “all scenarios not satisfying the criteria will be marked as exclude=True
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.
Hm yes, "modified" is not very specific. Modification of input objects can be dangerous in Python if users of the function are not aware of the behavior. Therefore when reading the word "modified" in the docstring, I'm warned as user, while in "marked as exclude=True
" I have to think a bit more what that means. I've tried to incorporate both thoughts into the next commit for the function. I think for the method it is less important to emphasize this, letting the method modify the object is more conventional in Python, I guess.
Thanks @lumbric for helping make our documentation better! @danielhuppmann is this good to go? |
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 @lumbric for this contribution!
Just a minor change, I'm not entirely sure if the second commit is really an improvement. I was a bit confused, because I didn't know if the function
validate()
would actually modify the object.Please let me know if you want me to drop the second commit or have other suggestions.