Skip to content

Make ActionGroup compatible with existing Parameter conventions#2505

Merged
j9ac9k merged 2 commits intopyqtgraph:masterfrom
ntjess:interactive-refactoring
Nov 1, 2022
Merged

Make ActionGroup compatible with existing Parameter conventions#2505
j9ac9k merged 2 commits intopyqtgraph:masterfrom
ntjess:interactive-refactoring

Conversation

@ntjess
Copy link
Copy Markdown
Contributor

@ntjess ntjess commented Oct 27, 2022

  • The name didn't end with Parameter which is unlike every other class
  • sigActivated should emit self instead of nothing
  • emitStateChanged should be fired during activate

- The name didn't end with `Parameter` which is unlike every other class
- `sigActivated` should emit `self` instead of nothing
- `emitStateChanged` should be fired during `activate`
Copy link
Copy Markdown
Member

@j9ac9k j9ac9k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ksunden
Copy link
Copy Markdown
Contributor

ksunden commented Oct 27, 2022

Any public API concerns from the name change?

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Oct 27, 2022

@ksunden This class was introduced in 0.13, so thankfully the API issue isn't binding at this point. Also, the parameter type is considered "private" (leading underscore), so even since the 0.13 release I don't think it would be widely adoptedmodified:

registerParameterType('_actiongroup', ActionGroupParameter, override=True)

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 27, 2022

I think we should probably leave the old class with a deprecation warning, saying it will be removed in the first pyqtgraph release after January, 2023. The old class, calling the "correct" named class. If it wasn't already in a release I would say we can just rename it, but it's been like this for long enough it might impact someone.

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Oct 27, 2022

Very understandable. Will do

@ntjess ntjess force-pushed the interactive-refactoring branch from 5923d8c to 830b71f Compare October 30, 2022 14:42
@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Oct 30, 2022

Good catch!

@ntjess
Copy link
Copy Markdown
Contributor Author

ntjess commented Nov 1, 2022

@j9ac9k Ok to merge?

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 1, 2022

whops, yes! Sorry! I'll press the buttan!

@j9ac9k j9ac9k merged commit 581c99b into pyqtgraph:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants