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

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Mar 11, 2017

Seemingly straightforward, needs further testing though.

Ended up reviving the NdElement deprecation PR here.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 12, 2017

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

This comment has been minimized.

Member

philippjfr commented Mar 12, 2017

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

philippjfr commented Mar 17, 2017

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

This comment has been minimized.

Member

jlstevens commented Mar 17, 2017

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

This comment has been minimized.

Member

philippjfr commented Mar 17, 2017

Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 17, 2017

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 from Dropped NdElement as baseclass for Collator to Avoid dependence on NdElement and add deprecation warning Mar 17, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 18, 2017

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

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 78.574%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the collator_wout_ndelement branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment