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

Avoid dependence on NdElement and add deprecation warning #1191

Merged
merged 4 commits into from
Mar 18, 2017

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Mar 11, 2017

Seemingly straightforward, needs further testing though.

Ended up reviving the NdElement deprecation PR here.

@jlstevens
Copy link
Contributor

Ended up reviving the NdElement deprecation PR here.

If you are trying to deprecate NdElement entirely, then I fully support that! The title of this PR will need to be updated though...

@philippjfr
Copy link
Member Author

If you are trying to deprecate NdElement entirely, then I fully support that! The title of this PR will need to be updated though...

That is the idea yes, the problem is pickle compatibility which would be a real pain.

@jlstevens
Copy link
Contributor

jlstevens commented Mar 13, 2017

the problem is pickle compatibility which would be a real pain.

Agreed. For this reason if you do get pickle compatibility working, make sure to add a deprecation warning so that when the time comes, we are less likely to forget to strip out the (inevitably!) horrible pickling hacks.

@philippjfr
Copy link
Member Author

As outlined in #1146, this PR now stops NdElement being used by default under any circumstances, with an additional deprecation warning issued when it an NdElement is instantiated for an unforeseen reason. In v2.0 NdElements will be removed and I'd even support dropping pickle support.

@jlstevens
Copy link
Contributor

In v2.0 NdElements will be removed and I'd even support dropping pickle support.

Agreed. Version 2.0 is also there to allow us to clean up things like this and make a break with the 1.x series. We should consider backporting fixes from 2.0 into 1.7.x and if we find there is demand for continued 1.0 support, we should consider backporting key features from 2.0 into a 1.8 release (after 2.0).

@philippjfr
Copy link
Member Author

Ready to merge.

@jlstevens
Copy link
Contributor

The title should still be updated as I think it is now more general than what is suggested.

I assume I'm not seeing any __getstate__ or __setstate__ magic because the class is still there and if unpickled will warn? I'm not entirely sure that is true as I can't remember if pickle works via the constructor or not...

@philippjfr philippjfr changed the title Dropped NdElement as baseclass for Collator Avoid dependence on NdElement and add deprecation warning Mar 17, 2017
@jlstevens
Copy link
Contributor

In discussion we decided that there would be plenty of warnings when working with NdElement (due to clone) even if the unpickling step itself doesn't warn.

Merging.

@jlstevens jlstevens merged commit df160aa into master Mar 18, 2017
@philippjfr philippjfr deleted the collator_wout_ndelement branch April 11, 2017 12:29
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants