Proposed fix for issue #50 #51
Conversation
|
I know that some tests are failing, but they did fail (on my machine at least) on branch master as well, so I do not think that this is due to my patch... |
| if options['strict']: | ||
| raise ValueError("Unrecognized key %s" % key) | ||
| elif droppedKeys is not None: | ||
| droppedKeys.add(key) |
There was a problem hiding this comment.
Returning a value from a function by modifying an argument is quite an unpopular method with Python programmers. This is a side effect; I would discourage it.
There was a problem hiding this comment.
@anatoly-scherbakov could you refactor this in a way that still fixes the issue?
There was a problem hiding this comment.
@anatoly-scherbakov I disagree that it is inherently a bad thing for a function to mutate one of its arguments, as long as the contract is clear about it.
That being said, I agree that having this "output argument" included in the "options" argument (which is clearly an input argument otherwise), is not great.
I don't consider that changing the return value of expand, in order to include the information about dropped keys, is desirable (it would break existing code).
I believe that a better solution to be to add a 4th parameter to expand: dropped=None. If that parameter is explicitly specified, it must be an (empty?) list, and the contract would be that this list would be populated with the dropped keys.
There was a problem hiding this comment.
Even if the contract is explicitly specified, this is not what a Python developer is expecting from a library, thus violating the Principle of Least Astonishment.
I have proposed a PR: #186 — but its not ready for merge as it should be really targeted at another PR of mine.
No description provided.