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

PendingDeprecationWarning instead of DeprecationWarning #2491

Closed
RMeli opened this issue Feb 1, 2020 · 8 comments
Closed

PendingDeprecationWarning instead of DeprecationWarning #2491

RMeli opened this issue Feb 1, 2020 · 8 comments

Comments

@RMeli
Copy link
Member

RMeli commented Feb 1, 2020

I was looking at #2472 and found out that there are two sub-packages for hydrogen bond analysis: MDAnalysis/analysis/hbond and MDAnalysis/analysis/hydrogenbonds. In MDAnalysis/analysis/hbond there is this warning:

warnings.warn(
            "This module will be deprecated in version 1.0."
            "Please use MDAnalysis.analysis.hydrogenbonds.hbond_analysis instead.",
            category=DeprecationWarning
        )

I think a PendingDeprecationWarning is more appropriate in this case.

@IAlibay
Copy link
Member

IAlibay commented Feb 1, 2020

This is code that is in progress of getting removed as part of #2443 , but I believe all previous cases of deprecations have been calling DeprecationWarning. A change might not be a bad idea though.

@RMeli
Copy link
Member Author

RMeli commented Feb 1, 2020

Should it be removed? The message says that it will be deprecated in version 1.0, not removed...

@IAlibay
Copy link
Member

IAlibay commented Feb 1, 2020

That is a very good point, I hadn't read it properly it seems.

Maybe one of the coredevs can clarify here, I'm glad I hadn't pushed part 2 of my removals yet 😅

@RMeli
Copy link
Member Author

RMeli commented Feb 1, 2020

I think if we are going for 0.21 it should be changed to PendingDeprecationWarning, while if we are going for 1.0 we should simply change the error message to something like this:

warnings.warn(
            "This module is deprecated."
            "Please use MDAnalysis.analysis.hydrogenbonds.hbond_analysis instead.",
            category=DeprecationWarning
        )

But removing code is always good, so maybe it can be removed directly in 1.0 since a DeprecationWarning was already in place?

@IAlibay
Copy link
Member

IAlibay commented Feb 1, 2020

Yes, I agree, I'll add the change assuming v1.0 as part of the removal changes.

(the alternative code seems like it was added in v0.21 so it probably is best to just go for a proper deprecation).

@IAlibay IAlibay mentioned this issue Feb 2, 2020
5 tasks
@orbeckst
Copy link
Member

orbeckst commented Feb 3, 2020

In this case PendingDeprecationWarning seems more appropriate than DeprecationWarning. Is this an existing warning in 2 and 3?

1.0 will contain both hbonds modules as we want to make it easy for users to keep the last 2-capable version of MDA for their projects.

For 2.0 we would want to remove hbonds — if we can.

@RMeli
Copy link
Member Author

RMeli commented Feb 3, 2020

Yes, PendingDeprecationWarning is also available in Python 2.7. I can change it to PendingDeprecationWarning if there is a version 0.21; otherwise I'll live it as it is for 1.0.

I checked other deprecation warnings and this is the only one where "pending" is more appropriate.

@orbeckst
Copy link
Member

orbeckst commented Feb 3, 2020 via email

@RMeli RMeli closed this as completed Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants