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

Deprecate lib.util.which #4340

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Deprecate lib.util.which #4340

merged 3 commits into from
Nov 6, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Nov 5, 2023

Fixes #3649

Notes:

  • Not switching over x3dna because that code doesn't even work.
  • Separate PR has been opened in hole2-mdakit

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


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

@pep8speaks
Copy link

pep8speaks commented Nov 5, 2023

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

Line 35:1: E402 module level import not at top of file

Comment last updated at 2023-11-05 12:37:53 UTC

Copy link

github-actions bot commented Nov 5, 2023

Linter Bot Results:

Hi @IAlibay! 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 ⚠️ Possible failure

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/6761340134/job/18376120039


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

Copy link

codecov bot commented Nov 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cb26ebc) 93.37% compared to head (3914925) 93.36%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4340      +/-   ##
===========================================
- Coverage    93.37%   93.36%   -0.02%     
===========================================
  Files          170      184      +14     
  Lines        22299    23410    +1111     
  Branches      4075     4075              
===========================================
+ Hits         20822    21856    +1034     
- Misses         962     1036      +74     
- Partials       515      518       +3     
Files Coverage Δ
package/MDAnalysis/analysis/hole2/hole.py 83.11% <100.00%> (+0.04%) ⬆️
package/MDAnalysis/lib/util.py 89.14% <100.00%> (-0.51%) ⬇️

... and 14 files with indirect coverage changes

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

@@ -228,7 +229,7 @@ def hole(pdbfile,
warnings.warn(msg.format(output_level))

# get executable
exe = util.which(executable)
exe = shutil.which(executable)
Copy link
Member Author

Choose a reason for hiding this comment

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

not looking to add tests to cover these if the migration is also being done in hole2-mdakit

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @IAlibay

@IAlibay IAlibay merged commit 21a15e3 into develop Nov 6, 2023
21 of 22 checks passed
@IAlibay IAlibay deleted the which branch November 6, 2023 08:31
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.

Replace custom lib.util.which with shutil.which
3 participants