-
-
Notifications
You must be signed in to change notification settings - Fork 373
Deprecate group() #3435
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
base: main
Are you sure you want to change the base?
Deprecate group() #3435
Conversation
|
👍 |
9ee608b to
52f2c89
Compare
# Conflicts: # docs/developers/contributing.rst # docs/quickstart.rst # docs/user-guide/groups.rst
e7370af to
8c85c4b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3435 +/- ##
==========================================
- Coverage 60.90% 60.88% -0.02%
==========================================
Files 86 86
Lines 10174 10176 +2
==========================================
Hits 6196 6196
- Misses 3978 3980 +2
🚀 New features to boost your workflow:
|
| return z | ||
|
|
||
|
|
||
| @deprecated("Use open_group() or create_group() instead") |
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.
Should we state that group will be removed in a future release (3.2?)
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'm happy either way. If you'd like me to add a specific release, let me know, and I can update the developer docs too with instructions to do this when deprecating.
d-v-b
left a comment
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.
just 1 question about whether we should be more specific in the deprecation warning, otherwise looks good
Recently #3406 was a bit of a tangle where
group()andcreate_group()had different behaviour - this was fixed by makinggroup()a simple wrapper ofcreate_group(), as intended.To reduce this API surface, this PR deprecates
group(), advising users to use eithercreate_group()oropen_group(). The intent of these other functions is clearer, and it mirrors the methods on theGroupclass, which does not have agroup()method.I left in one test that uses
group()to make sure it still works during the deprecation period.TODO:
docs/user-guide/*.rstchanges/