Conversation
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of changes, probably would be worth switching over to the deprecate decorator. Also we'll need to test warnings (or at least fix the test for warnings).
| import logging | ||
| logger = logging.getLogger("MDAnalysis.analysis.helanal") | ||
|
|
||
| warnings.warn("This module is deprecated as of MDAnalysis version 1.0. " |
There was a problem hiding this comment.
We'll probably want some test for this? (not sure how strict we are being with 1.0.1 tests, but if it's getting released we should try to get coverage down?)
There was a problem hiding this comment.
Actually the tests are failing here, so we it'll need fixing anyways.
|
Just fyi, you can also make a PR against master, that's fine, too. |
|
Hello @lilyminium! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-09-04 07:33:03 UTC |
lilyminium
force-pushed
the
deprecate-helanal
branch
from
e79c58e to
f1810ee
Compare
Codecov Report
@@ Coverage Diff @@
## backport-1.0.1 #2928 +/- ##
==================================================
- Coverage 92.00% 91.80% -0.21%
==================================================
Files 169 15 -154
Lines 23175 2025 -21150
Branches 3186 0 -3186
==================================================
- Hits 21322 1859 -19463
+ Misses 1760 166 -1594
+ Partials 93 0 -93 Continue to review full report at Codecov.
|
|
Thank you @lilyminium and thanks @IAlibay for the review! |
* deprecate helanal, to be removed in 2.0.0 (use analysis.helix_analysis in 2.0 from PR #2622) * add deprecation warnings to analysis.helanal * update CHANGELOG
fiona-naughton
added
maintainability
Component-Analysis
deprecation
6 participants
Changes made in this Pull Request:
Sorry for the delay in adding the helanal deprecation notice.