Fix a typo in a log message and extend a docstring #235
Conversation
There was a problem hiding this comment.
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.
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.
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? |
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.