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

Add NaTType to tslib public api #16137

Closed
wants to merge 1 commit into from

Conversation

dhirschfeld
Copy link
Contributor

When testing the new 0.20.0 release candidate I found that the removal of NaTType from the tslib public api breaks odo.

If this api breakage is actually intentional/desired I'm ok with this being closed however it should probably get a mention in the backwards incompatible API changes section of the whatsnew.

Whilst a deprecation warning is raised in 648ae4f#diff-d6b2c5dbe1e7fbda4ed4b87e0c49399f the removal of NaTType breaks the previous public api rendering the warning ineffective and not giving any 3rd party libraries a chance to make the necessary changes and cut a release

@codecov
Copy link

codecov bot commented Apr 26, 2017

Codecov Report

Merging #16137 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #16137   +/-   ##
=======================================
  Coverage   90.84%   90.84%           
=======================================
  Files         159      159           
  Lines       50779    50779           
=======================================
  Hits        46128    46128           
  Misses       4651     4651
Flag Coverage Δ
#multiple 88.62% <ø> (ø) ⬆️
#single 40.33% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/tslib.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d50b162...06eafff. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 26, 2017

Codecov Report

Merging #16137 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #16137   +/-   ##
=======================================
  Coverage   90.84%   90.84%           
=======================================
  Files         159      159           
  Lines       50779    50779           
=======================================
  Hits        46128    46128           
  Misses       4651     4651
Flag Coverage Δ
#multiple 88.62% <ø> (ø) ⬆️
#single 40.33% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/tslib.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d50b162...06eafff. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented Apr 26, 2017

NaTType was never a public api
who/what is using this?

@dhirschfeld
Copy link
Contributor Author

odo was using that

@jorisvandenbossche
Copy link
Member

We could maybe expose NaTType in pandas.api.types ?

@jorisvandenbossche
Copy link
Member

Depending on what we decide (whether we find it worth to expose NaTType publicly or not), you also have to adapt __init__.py here: https://github.com/pandas-dev/pandas/blob/master/pandas/__init__.py#L76 to provide the correct deprecation message.

@dhirschfeld
Copy link
Contributor Author

It would be ok for odo to use a private attribute if they're aware that it is meant to be private which the new import from pd._libs.tslib would indicate.

It would be nice I guess if they didn't have to though so I'd be in favour of exposing it in pd.api.types.

@jreback
Copy link
Contributor

jreback commented Apr 26, 2017

superseded by #16146

@jreback jreback closed this Apr 26, 2017
@shoyer
Copy link
Member

shoyer commented Apr 26, 2017

The right way to use this going forward is probably type(pd.NaT). I'm not sure what the point of exposing NaTType is, given that it's a singleton. Python 2 had types.NoneType but they removed it in Python 3.

jreback added a commit to jreback/pandas that referenced this pull request Apr 27, 2017
pcluo pushed a commit to pcluo/pandas that referenced this pull request May 22, 2017
@dhirschfeld dhirschfeld deleted the patch-1 branch January 29, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants