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

DEPR: msgpack #30112

Merged
merged 17 commits into from
Dec 12, 2019
Merged

DEPR: msgpack #30112

merged 17 commits into from
Dec 12, 2019

Conversation

jbrockmendel
Copy link
Member

there are two msgpack files in LICENSES, should those go?

@jreback jreback added Deprecate Functionality to remove in pandas IO Msgpack labels Dec 6, 2019
@jreback jreback added this to the 1.0 milestone Dec 6, 2019
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

big +1. ping on green.

@jbrockmendel
Copy link
Member Author

could use some help on the whatsnew 0.13.0 file

@WillAyd
Copy link
Member

WillAyd commented Dec 6, 2019

I would think OK to just remove that section of the whatsnew

@jreback
Copy link
Contributor

jreback commented Dec 6, 2019

could use some help on the whatsnew 0.13.0 file

can you leave it as a code-block?

@jbrockmendel
Copy link
Member Author

can you leave it as a code-block?

leaving it as-is I get errors in the docbuild bc the msgpack funcs arent defined.

.. _io.msgpack:

msgpack
-------
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is deprecated/removed on a very short time frame, I think it might be good to keep this title a bit longer with a small note that it was deprecated/removed and how to replace it.

Copy link
Contributor

Choose a reason for hiding this comment

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

why? the whole point is to clean things for 1.0

Copy link
Member

Choose a reason for hiding this comment

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

To provide documentation about this removal, and how to replace it.

For example, there have been issues opened with questions about this which lead to added examples in the docstrings, but this has never been published yet in actual documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

k that’s fair

Copy link
Member Author

Choose a reason for hiding this comment

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

restored this section. LMK if you have suggestions for additional notes to put in here.

any thoughts on the LICENSE files mentioned in the OP?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't restore the full section, but rather only keep a short explanation that it was deprecated/removed, and how to replace it (see the docstrings for some content for that)

@jorisvandenbossche
Copy link
Member

there are two msgpack files in LICENSES, should those go?

I suppose so yes? If we don't keep any code of it, we can remove the licenses as well?

@jreback
Copy link
Contributor

jreback commented Dec 11, 2019

there are two msgpack files in LICENSES, should those go?

I suppose so yes? If we don't keep any code of it, we can remove the licenses as well?

yes for sure

@jreback
Copy link
Contributor

jreback commented Dec 11, 2019

pls rebase

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

small change. ping on green.

:okwarning:

pd.read_msgpack(df.to_msgpack() + s.to_msgpack())
pandas support for ``msgpack`` has been removed in version 1.0.0. It is recommended to use pyarrow for on-the-wire transmission of pandas objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

we currently have a small code snippet in the doc-string of how to use pyarrow, can you copy it here as a code-block

pd.read_msgpack(df.to_msgpack() + s.to_msgpack())
For documentation on pyarrow, see `here<https://arrow.apache.org/docs/python/index.html>`__.
Copy link
Member Author

Choose a reason for hiding this comment

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

i think the docbuild is complaining about this. help @jorisvandenbossche ?

Copy link
Member

@WillAyd WillAyd Dec 11, 2019

Choose a reason for hiding this comment

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

Suggested change
For documentation on pyarrow, see `here<https://arrow.apache.org/docs/python/index.html>`__.
For documentation on pyarrow, see `here <https://arrow.apache.org/docs/python/index.html>`__.

Copy link
Member

Choose a reason for hiding this comment

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

I think the double underscore is the issue

Copy link
Member

Choose a reason for hiding this comment

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

The double underscore is correct, but I suspect the space that you corrected is the problem though

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like that fixed it, thanks

@jreback
Copy link
Contributor

jreback commented Dec 12, 2019

lgtm. @jorisvandenbossche

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looks good!

@WillAyd WillAyd merged commit 4b9c62d into pandas-dev:master Dec 12, 2019
@WillAyd
Copy link
Member

WillAyd commented Dec 12, 2019

Thanks @jbrockmendel !

@jbrockmendel jbrockmendel deleted the depr-msgpack branch December 12, 2019 16:23
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants