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

Unpin pycodestyle #25789

Merged
merged 10 commits into from
Mar 20, 2019
Merged

Unpin pycodestyle #25789

merged 10 commits into from
Mar 20, 2019

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Mar 19, 2019

Larger change than I would have hoped for but it's tough to split this up given the typing import errors get bundled in with F821 errors during LINTing that we certainly wouldn't want to ignore

@@ -2,4 +2,4 @@
# flake8: noqa

from .tslibs import (
iNaT, NaT, Timestamp, Timedelta, OutOfBoundsDatetime, Period)
iNaT, NaT, NaTType, Timestamp, Timedelta, OutOfBoundsDatetime, Period)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if there's any objection to doing this, but if we didn't want to expose this would need to update type comments @jbrockmendel

Copy link
Member

Choose a reason for hiding this comment

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

whats the motivation here? I wouldn't want to expose this ideally. But its explicitly private so not really harmful.

):
# type: (...) -> PeriodArray

# TODO: ABCIndexClass is a valid type for other but had to be excluded
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment explains it, but just to clarify this PR doesn't necessarily check that comments are correct as much as gets them to pass linting. I figure it makes sense to validate the comments themselves when moving over to Py3 syntax, so this was the quick way of getting this to pass

@@ -440,7 +440,6 @@ def _chk_truncate(self):
max_rows = self.max_rows

if max_cols == 0 or max_rows == 0: # assume we are in the terminal
# (why else = 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

Hanging comment was causing a lint error. I figured it didn't add much so easier to remove than reword

@@ -201,7 +201,8 @@ def test_set_index_pass_arrays_duplicate(self, frame_of_index_cols, drop,
# need to adapt first drop for case that both keys are 'A' --
# cannot drop the same column twice;
# use "is" because == would give ambiguous Boolean error for containers
first_drop = False if (keys[0] is 'A' and keys[1] is 'A') else drop
first_drop = False if (
keys[0] is 'A' and keys[1] is 'A') else drop # noqa: F632
Copy link
Member Author

Choose a reason for hiding this comment

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

F632 is ignored because it calls out using is for equality comparisons. You can see in the note above though that this test intentionally uses is instead of the equality operator

Copy link
Member

Choose a reason for hiding this comment

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

IIRC interning of short strings is a CPython implementation detail. Are we sure we want to rely on that?

@@ -22,6 +22,7 @@ def test_namespace():
'timezones']

api = ['NaT',
'NaTType',
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was failing because of NaTType addition to API so simply added here @jbrockmendel

@WillAyd WillAyd added CI Continuous Integration Code Style Code style, linting, code_checks Dependencies Required and optional dependencies labels Mar 19, 2019
@@ -365,7 +367,7 @@ def isna(self):
raise AbstractMethodError(self)

def _values_for_argsort(self):
# type: () -> ndarray
# type: () -> np.ndarray
Copy link
Contributor

Choose a reason for hiding this comment

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

we can switch to the py3 style now i think (here or later)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea planning to do later to minimize diff

Copy link
Contributor

Choose a reason for hiding this comment

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

great

@@ -461,10 +462,10 @@ def __getitem__(self, key):
def __setitem__(
self,
key, # type: Union[int, Sequence[int], Sequence[bool], slice]
value, # type: Union[NaTType, Scalar, Sequence[Scalar]]
value, # type: Union[NaTType, Any, Sequence[Any]]
Copy link
Contributor

Choose a reason for hiding this comment

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

does Any not encompan NaTType here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid question. I mostly changed this to get it to pass since Scalar isn't a real thing as indicated by subsequent TODO. Think I should just remove Any altogether for the time being?

Copy link
Contributor

Choose a reason for hiding this comment

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

no it would fail for that. What we need is a type that is an allowed datetime value, e.g: string, datetime / Timestamp, NaT). These can of course be done later, but should start setting up a pandas.typing i think.

Copy link
Member

Choose a reason for hiding this comment

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

Yah, I'd rather have an import from pandas.typing and avoid imports that aren't "really" used (i.e. NaTType).

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #25789 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25789      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         172      172              
  Lines       52965    52982      +17     
==========================================
+ Hits        48337    48354      +17     
  Misses       4628     4628
Flag Coverage Δ
#multiple 89.83% <100%> (ø) ⬆️
#single 41.76% <95.83%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/integer.py 96.32% <ø> (ø) ⬆️
pandas/io/formats/format.py 97.99% <ø> (ø) ⬆️
pandas/_libs/__init__.py 100% <ø> (ø) ⬆️
pandas/_libs/tslibs/__init__.py 100% <100%> (ø) ⬆️
pandas/core/dtypes/dtypes.py 96.1% <100%> (ø) ⬆️
pandas/core/arrays/base.py 98.27% <100%> (+0.02%) ⬆️
pandas/core/arrays/datetimes.py 97.8% <100%> (ø) ⬆️
pandas/core/common.py 98.39% <100%> (ø) ⬆️
pandas/core/arrays/period.py 98.53% <100%> (ø) ⬆️
pandas/core/groupby/groupby.py 97.21% <100%> (ø) ⬆️
... and 9 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 8e54e55...9d4ece7. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #25789 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25789      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         172      172              
  Lines       52965    52982      +17     
==========================================
+ Hits        48337    48354      +17     
  Misses       4628     4628
Flag Coverage Δ
#multiple 89.83% <100%> (ø) ⬆️
#single 41.76% <95.83%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/integer.py 96.32% <ø> (ø) ⬆️
pandas/io/formats/format.py 97.99% <ø> (ø) ⬆️
pandas/_libs/__init__.py 100% <ø> (ø) ⬆️
pandas/_libs/tslibs/__init__.py 100% <100%> (ø) ⬆️
pandas/core/dtypes/dtypes.py 96.1% <100%> (ø) ⬆️
pandas/core/arrays/base.py 98.27% <100%> (+0.02%) ⬆️
pandas/core/arrays/datetimes.py 97.8% <100%> (ø) ⬆️
pandas/core/common.py 98.39% <100%> (ø) ⬆️
pandas/core/arrays/period.py 98.53% <100%> (ø) ⬆️
pandas/core/groupby/groupby.py 97.21% <100%> (ø) ⬆️
... and 9 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 8e54e55...739bcab. Read the comment docs.

@jreback jreback added this to the 0.25.0 milestone Mar 19, 2019
@jreback
Copy link
Contributor

jreback commented Mar 19, 2019

@WillAyd ok with merging as-is if this is easier. before we start typing in earnest should convert the old style types & then consider making some custom-ish types (datetime)

@WillAyd
Copy link
Member Author

WillAyd commented Mar 20, 2019

I'd prefer to merge as is but @jbrockmendel if you think we need to edit some things before let me know.

I think defining a TypeVar for allowed date time values is a good idea and can start momentum on an actual typing module, so happy to open that as a follow up

@jbrockmendel
Copy link
Member

Not a big deal, go for it

@jreback
Copy link
Contributor

jreback commented Mar 20, 2019

@WillAyd ok this is great. Let's add (if not already on your list)

  1. custom types for datetimes + nats (Datetime?)
  2. custom type for Union[Datetime, str] (or maybe can just be explict about that)
  3. code_checks / mypy to make sure we don't have py2 style types.
  4. add mypy checks on CI & pycodestyle as a testing dep

@jreback jreback merged commit 05780f8 into pandas-dev:master Mar 20, 2019
@WillAyd WillAyd deleted the unpin-pycodestyle branch March 20, 2019 01:23
sighingnow added a commit to sighingnow/pandas that referenced this pull request Mar 20, 2019
* origin/master:
  DOC: clean bug fix section in whatsnew (pandas-dev#25792)
  DOC: Fixed PeriodArray api ref (pandas-dev#25526)
  Move locale code out of tm, into _config (pandas-dev#25757)
  Unpin pycodestyle (pandas-dev#25789)
  Add test for rdivmod on EA array (GH23287) (pandas-dev#24047)
  ENH: Support datetime.timezone objects (pandas-dev#25065)
  Cython language level 3 (pandas-dev#24538)
  API: concat on sparse values (pandas-dev#25719)
  TST: assert_produces_warning works with filterwarnings (pandas-dev#25721)
  make core.config self-contained (pandas-dev#25613)
  CLN: replace %s syntax with .format in pandas.io.parsers (pandas-dev#24721)
  TST: Check pytables<3.5.1 when skipping (pandas-dev#25773)
  DOC: Fix typo in docstring of DataFrame.memory_usage  (pandas-dev#25770)
  Replace dicts with OrderedDicts in groupby aggregation functions (pandas-dev#25693)
  TST: Fixturize tests/frame/test_missing.py (pandas-dev#25640)
  DOC: Improve the docsting of Series.iteritems (pandas-dev#24879)
  DOC: Fix function name. (pandas-dev#25751)
  Implementing iso_week_year support for to_datetime (pandas-dev#25541)
  DOC: clarify corr behaviour when using a callable (pandas-dev#25732)
  remove unnecessary check_output (pandas-dev#25755)

# Conflicts:
#	doc/source/whatsnew/v0.25.0.rst
thoo added a commit to thoo/pandas that referenced this pull request Mar 20, 2019
* upstream/master: (55 commits)
  PERF: Improve performance of StataReader (pandas-dev#25780)
  Speed up tokenizing of a row in csv and xstrtod parsing (pandas-dev#25784)
  BUG: Fix _binop for operators for serials which has more than one returns (divmod/rdivmod). (pandas-dev#25588)
  BUG-24971 copying blocks also considers ndim (pandas-dev#25521)
  CLN: Panel reference from documentation (pandas-dev#25649)
  ENH: Quoting column names containing spaces with backticks to use them in query and eval. (pandas-dev#24955)
  BUG: reading windows utf8 filenames in py3.6 (pandas-dev#25769)
  DOC: clean bug fix section in whatsnew (pandas-dev#25792)
  DOC: Fixed PeriodArray api ref (pandas-dev#25526)
  Move locale code out of tm, into _config (pandas-dev#25757)
  Unpin pycodestyle (pandas-dev#25789)
  Add test for rdivmod on EA array (GH23287) (pandas-dev#24047)
  ENH: Support datetime.timezone objects (pandas-dev#25065)
  Cython language level 3 (pandas-dev#24538)
  API: concat on sparse values (pandas-dev#25719)
  TST: assert_produces_warning works with filterwarnings (pandas-dev#25721)
  make core.config self-contained (pandas-dev#25613)
  CLN: replace %s syntax with .format in pandas.io.parsers (pandas-dev#24721)
  TST: Check pytables<3.5.1 when skipping (pandas-dev#25773)
  DOC: Fix typo in docstring of DataFrame.memory_usage  (pandas-dev#25770)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Code Style Code style, linting, code_checks Dependencies Required and optional dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Pycodestyle Pin
3 participants