-
Notifications
You must be signed in to change notification settings - Fork 1
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 (GT-96): parameters tab search past first #15
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
d9c87b8
fix (GT-96): parameters tab search past first
gordon-epc 8949407
fix (GT-96): add typing to modified classes, add base class
gordon-epc f542afb
fix (GT-96): update to appease black
gordon-epc e505610
fix (GT-96): columns attr.ib definition with converter
gordon-epc 821d72c
fix (GT-96): search_view better index
gordon-epc be4146c
fix (GT-96): message_handler prints stack trace
gordon-epc 64e4243
fix (GT-96): once again, update to appease black
gordon-epc a993444
fix (GT-96): undo inheritance for proxy models
gordon-epc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
(tl;dr probably do nothing, but I felt like thinking about this so I wrote this up anyways)
So, I just learned that you can pass properties to the constructors with PyQt (python-qt-tools/PyQt5-stubs#147 (comment)). I don't know that I want to use that, or if it works with PySide (maybe relevant someday here, definitely relevant elsewhere for us), but figured I'd share anyways. Point is that this could get moved up to the attribute definition then.
But that still doesn't get rid of the
.__attrs_post_init__()
so... whatever... until we write a qt-specific attrs decorator... someday. Or maybe your change below leads somewhere good. Taking it a step further, perhaps make a basic base class that handles the.__attrs_post_init__()
then leave all the other classes clean (at least those that only need parent passed up).Note the 'private'
._parent
to avoid shadowing theQObject.parent()
method. Yeah, that issue pre-exists this change, just noticed it. Ah, inheritance...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 held off changing the
wildcard
declaration + initialization to a one liner. I did do the_parent
. Again that's a good find.What I found is that no matter what, there is no way to avoid adding the
__attrs_post_init__
method. The inherited class init method has to be called and as far as I can tell the only way to do that is making a call through__attrs_post_init__
.Really, it doesn't make much sense to have the base class. It's not saving any lines of code, that's for sure. Let me know what you think.
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.
Having a general
AttrsQObject
, or whatever, is a bigger picture thing than this PR or these two classes. The sort of thing that is of at least uncertain value to backport, but ... might (?) be a good thing for future development. It would clearly avoid the.parent
shadow issue in one place and allow perhaps many other classes to not have a.__attrs_post_init__()
which would encourage them to do less in there which is one of the things I've accepted as good about attrs. No more, 'well I already have one so... do a bit more!'.:]
But yeah, big picture, various opinions, not really to be dealt with in this PR.