Skip to content
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

Add DepWarnings to X3DNA code and docs #4333

Merged
merged 5 commits into from
Nov 5, 2023

Conversation

hmacdope
Copy link
Member

@hmacdope hmacdope commented Nov 5, 2023

Fixes #3788

Changes made in this Pull Request:

  • Adds deprecation warnings to X3DNA as slated for removal in 3.0

PR Checklist

  • [ X] Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4333.org.readthedocs.build/en/4333/

@pep8speaks
Copy link

pep8speaks commented Nov 5, 2023

Hello @hmacdope! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 143:72: W291 trailing whitespace
Line 171:80: E501 line too long (125 > 79 characters)
Line 491:12: E128 continuation line under-indented for visual indent
Line 491:72: W291 trailing whitespace
Line 740:12: E128 continuation line under-indented for visual indent
Line 740:72: W291 trailing whitespace

Comment last updated at 2023-11-05 08:51:21 UTC

Copy link

github-actions bot commented Nov 5, 2023

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.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/6760288579/job/18373890117


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

"""
warnings.warn("X3DNA module is deprecated and will be removed in MDAnalysis 3.0, see #3788", category=DeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for testing this locally, that's probably good enough.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Thanks @hmacdope - I've got two comments here:

  1. I believe someone at some point mentioned this code wasn't even usable - can you actually get these warnings if you try to use the method / class locally?
  2. Please use the relevant decorator for the method.

"""
warnings.warn("X3DNA module is deprecated and will be removed in MDAnalysis 3.0, see #3788", category=DeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

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

we have a decorator for this - please see

@deprecate(release="2.6.0", remove="3.0.0",
message=("This method has been moved to the MDAKit hole2-mdakit: "
"https://github.com/MDAnalysis/hole2-mdakit"))
for an example

Copy link
Member Author

Choose a reason for hiding this comment

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

ah ta.

Copy link
Member Author

Choose a reason for hiding this comment

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

RE: 1 if the code is not usable and we are removing it i don't really want to invest any energy in finding out why / how. Is it okay just to put the warnings in as is? Let me know wither way.

Copy link
Member

Choose a reason for hiding this comment

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

@hmacdope - my point here is "what's the point in putting in a warning if it won't even work locally". If you can get the warnings locally then it's fine, otherwise I'd just add in the docstring changes and call it a day.

Copy link
Member Author

@hmacdope hmacdope Nov 5, 2023

Choose a reason for hiding this comment

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

@IAlibay Okay you can get the warnings on before whatever is broken, at least it may warn the (possibly 0) people that have a niche setup that this works for this that it will be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for testing this locally, that's probably good enough.

Copy link

codecov bot commented Nov 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5fa0d27) 93.37% compared to head (a7615ce) 93.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##           develop    #4333     +/-   ##
==========================================
  Coverage    93.37%   93.37%             
==========================================
  Files          170      184     +14     
  Lines        22295    23403   +1108     
  Branches      4075     4075             
==========================================
+ Hits         20818    21853   +1035     
- Misses         962     1035     +73     
  Partials       515      515             

see 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IAlibay
Copy link
Member

IAlibay commented Nov 5, 2023

thanks @hmacdope !

@IAlibay IAlibay merged commit 540a980 into MDAnalysis:develop Nov 5, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove x3dna in 3.0 (Unpacking errors in x3dna)
3 participants