-
Notifications
You must be signed in to change notification settings - Fork 22
feat: add signal aliases on SignalGroup #299
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
1453eb9
add signal_aliases
getzze 3282a6c
add test signal_aliases
getzze 7e848ca
add test signal_aliases
getzze b05eefd
add coverage
getzze 5b9a784
style(pre-commit.ci): auto fixes [...]
pre-commit-ci[bot] 52bec2f
clean
getzze 72a62de
compat py38
getzze 3d445a3
cover group.signals
getzze c128be3
more tests
tlambert03 a75bb53
Merge branch 'main' into signal-aliases
tlambert03 64e2fb4
try speedup evented setattr
tlambert03 4678ca4
Merge branch 'signal-aliases' of https://github.com/getzze/psygnal in…
tlambert03 02377b7
use type alias
tlambert03 49738d1
go back to optional str
tlambert03 aea60c7
simplify with_aliases
getzze 0c30d1f
revert with_aliases
getzze cfa8f71
remove from public
tlambert03 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 hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
hey @getzze, I think this is probably my only question (in an extremely nice PR). I can see why you did it this way as opposed to modifing
__getitem__It allows someone to query both the alias name and the original name. Do you think that's important? (I assume so, since you've been giving it a lot of thought and opted for this).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.
hey, I think it can be removed, I mean inlined in
evented_setattr, as it is only used there. AndSignalGrouphas one less method.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 do think that end-users should also be able to search by alias. but i guess the question is should they no longer be able to search by the original name? i.e. if I remove the method on SignalGroup, then end-users can no longer access the original names for an aliased signal, correct? are we ok with that?
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 made a function instead of a method, with a new name
get_signal_from_field.