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

CLN: De-privatize core.common funcs, remove unused #22001

Merged
merged 19 commits into from Jul 24, 2018

Conversation

Projects
None yet
4 participants
@jbrockmendel
Copy link
Member

commented Jul 20, 2018

Moves a few console-checking functions to io.formats.console.

A bunch of core.common functions were never used outside of tests, got rid of em.

The ones I left alone were _any_not_none, _all_not_none etc, as I'm inclined to think these should be removed in favor of python builtins.

@@ -386,7 +274,7 @@ def _get_callable_name(obj):
return None


def _apply_if_callable(maybe_callable, obj, **kwargs):
def apply_if_callable(maybe_callable, obj, **kwargs):

This comment has been minimized.

Copy link
@jreback

jreback Jul 20, 2018

Contributor

prob should move all of the indexing ones to a new indexing module

maybe pandas.core.indexing.utils
(and obviously move indexing.py itself)

returns True if running under python/ipython interactive shell
"""
from pandas import get_option

This comment has been minimized.

Copy link
@jreback

jreback Jul 20, 2018

Contributor

already imported

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jul 23, 2018

Author Member

In a different function, not at module level

This comment has been minimized.

Copy link
@jreback

jreback Jul 23, 2018

Contributor

ahh ok

@codecov

This comment has been minimized.

Copy link

commented Jul 20, 2018

Codecov Report

Merging #22001 into master will decrease coverage by <.01%.
The diff coverage is 90.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22001      +/-   ##
==========================================
- Coverage      92%      92%   -0.01%     
==========================================
  Files         170      170              
  Lines       50609    50551      -58     
==========================================
- Hits        46561    46507      -54     
+ Misses       4048     4044       -4
Flag Coverage Δ
#multiple 90.4% <89.63%> (-0.01%) ⬇️
#single 42.21% <44.55%> (+0.02%) ⬆️
Impacted Files Coverage Δ
pandas/plotting/_core.py 83.48% <0%> (ø) ⬆️
pandas/core/ops.py 96.11% <100%> (ø) ⬆️
pandas/core/indexes/numeric.py 97.24% <100%> (ø) ⬆️
pandas/core/strings.py 98.63% <100%> (ø) ⬆️
pandas/core/arrays/categorical.py 95.95% <100%> (ø) ⬆️
pandas/core/indexes/interval.py 94.12% <100%> (ø) ⬆️
pandas/core/indexes/api.py 98.91% <100%> (ø) ⬆️
pandas/core/indexes/base.py 96.37% <100%> (ø) ⬆️
pandas/core/indexes/category.py 97.28% <100%> (ø) ⬆️
pandas/core/indexes/datetimes.py 95.12% <100%> (ø) ⬆️
... and 34 more

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 171076c...8e5a1bd. Read the comment docs.

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 20, 2018

Hello @jbrockmendel! Thanks for updating the PR.

Line 103:5: E722 do not use bare except'
Line 121:5: E722 do not use bare except'
Line 140:5: E722 do not use bare except'
Line 152:5: E722 do not use bare except'

Comment last updated on July 24, 2018 at 04:05 Hours UTC

@jreback jreback added the Clean label Jul 20, 2018

@jreback jreback added this to the 0.24.0 milestone Jul 20, 2018

@jorisvandenbossche jorisvandenbossche changed the title [CLN] De-privatize core.common funcs, remove unused CLN: De-privatize core.common funcs, remove unused Jul 23, 2018

# ----------------------------------------------------------------------
# Detect our environment

def in_interactive_session():

This comment has been minimized.

Copy link
@jreback

jreback Jul 23, 2018

Contributor

AFAICT the 2nd 2 routines are not used at all? can you remove if that is the case

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2018

note pandas.core.common is technically public. We never made explicity made this non-public, so have to be careful about name changes here. I find these ok, but in the longer term we should probably change this.

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

note pandas.core.common is technically public. We never made explicity made this non-public, so have to be careful about name changes here.

Ah, yes, that's a good point. The ones we previously moved out we deprecated explicitly.

I don't think it would be too hard to deprecate all the renamed functions? (create the old ones calling the the new one + with added deprecation warning in a loop?)

@jbrockmendel

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2018

I'm unclear on how to move forward. If core.common is public, does that mean we need to not-remove the (deprecated) environment-detecting and other unused functions? Do we need to wrap+expose the currently-private functions?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

I think we only need to care about the changed non-private functions (e.g. no leading _).

Its fair game to move all of the console functions, as well as remove the unused functions. I don't think we should go crazy with deprecations. Maybe just add a blanket whatsnew. We only deprecated things because these were very commonly used function, all of the is_* ones (which moved to pandas.api.types).

I will have another look and indicate anything we actually need to deprecate.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

I am actually ok with these changes as-if (just add a whatsnew note). And add a disclaimer on the doc-string that this is a non-public module.

we long ago declared the pandas.core namespace private (it would break too much to actually change this to panda._core, though maybe we should actually do that.

@jreback jreback merged commit 2d0c961 into pandas-dev:master Jul 24, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

thanks @jbrockmendel

alimcmaster1 added a commit to alimcmaster1/pandas that referenced this pull request Aug 12, 2018

Sup3rGeo added a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.