-
Notifications
You must be signed in to change notification settings - Fork 652
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
Deprecate ts
argument to _read_next_timestep
#4334
base: develop
Are you sure you want to change the base?
Conversation
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.
Tests please
@@ -200,6 +200,9 @@ def _read_frame(self, i): | |||
|
|||
def _read_next_timestep(self, ts=None): | |||
"""copy next frame into timestep""" | |||
if ts: | |||
warnings.warn("ts argument to _read_next_timestep is deprecated as of 2.7.0 and will be removed in 3.0.0, see #3928") |
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.
Also these need to throw a deprecation warning rather than a (I believe by default) userwarning
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.
Yes very right.
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.
This is marked as resolved but looks like the change hasn't been pushed? I'm going to unresolve if that's ok.
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.
Sorry hallucinating on my end.
Linter Bot Results:Hi @hmacdope! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #4334 +/- ##
===========================================
- Coverage 93.37% 93.28% -0.10%
===========================================
Files 170 184 +14
Lines 22295 23433 +1138
Branches 4075 4089 +14
===========================================
+ Hits 20818 21859 +1041
- Misses 962 1047 +85
- Partials 515 527 +12
☔ View full report in Codecov by Sentry. |
@RMeli can I put you in charge to shepherd this PR to completion, please? If this doesn't work for you, please unassign yourself and let me know. Thanks! |
Sure @orbeckst. I think this is still blocked by review comments that have not yet been addressed. |
Yep sorry, just need to add tests and resolve review, short of time atm. |
@hmacdope is this PR something that you think you can finish up? |
Yes, apologies I will get to it soon. |
Fixes #3928
Changes made in this Pull Request:
_read_next_timestep
use of optionalts=None
arg (barely ever used)As this is an implementation detail I didn't add docs changes, as I couldn't see a record of it documented anywhere anyway.
The warning is guarded by a conditional to avoid an immense amount of warnings.
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4334.org.readthedocs.build/en/4334/