minor hotfix to repair Exporter and make Parameter more robust#2442
minor hotfix to repair Exporter and make Parameter more robust#2442Zorkator wants to merge 3 commits intopyqtgraph:masterfrom
Conversation
|
Thanks for the PR, but I'm curious -- What is the benefit of preventing |
| self.setValue(value) | ||
|
|
||
| if 'default' not in self.opts: | ||
| self.opts['default'] = None |
There was a problem hiding this comment.
I think I'm -1 on this change here (combined with using self.opts.get('default'). dict.get() is useful, but it can mask other bugs in code, and raising KeyError exceptions may contribute to "failing fast", which should significantly simplify debugging efforts.
There was a problem hiding this comment.
Thanks for your thoughts. Well, I see your point on "failing fast". However, here I'd rate it less than clearness of code and the promise of setDefault(): set the default.
As is, calling setDefault() requires making sure there's already some "default" (and which gets replaced), otherwise you risk a KeyError (which was actually my problem when calling it from my code).
In general, I think interface methods should do the complete job, in order to avoid the need of accessing the underlying data structure opts and using the correct key default, from outside again and again.
pyqtgraph/parametertree/Parameter.py
Outdated
| self.opts['limits'] = limits | ||
| self.sigLimitsChanged.emit(self, limits) | ||
| return limits | ||
| if not fn.eq(self.opts.get('limits'), limits): |
There was a problem hiding this comment.
If limits wasn't in opts before and the new value is None, sigLimitsChanged won't end up firing because self.opts.get('limits') will return None. The old behavior should be able to catch this
There was a problem hiding this comment.
Ah, you're right! ... somehow expecting same handling of key limits missing and set to None, since this is what happens with default (hasDefault() => False). So this change would change behaviour, sorry...
Hmm, wouldn't it be better (clearer) to handle all options the same way? But this would surely require more changes throughout the code :-/. So, better revert this for now.
…ot handled the same.
|
I'm going to close this PR and say it's super-seeded by #2758 which is just the first commit of this PR. As I said in the comments, I'm not willing to merge the PR here in its current form in part due to the red CI, but until there is a good case why the library should make more use of dict.get() vs allowing for If there is a bug that is fixed by using Thanks for the PR, sorry it took so long to get it (partially) merged |
No description provided.