Skip to content

Conversation

@jschendel
Copy link
Member

@jschendel jschendel commented Mar 30, 2018

Noticed some lgtm.com alerts about the @property decorator not working in old-style classes in scripts/validate_docstrings.py. I don't think this actually causes any undesired behavior in that script though.

Figured this would be a good time to enforce new-style classes throughout the codebase:

  • Converted old-style classes to new-style classes
    • Basically just inserted (object).
  • Added a check to lint.sh for old-style classes, since flake8 doesn't look to be catching it.
    • Maybe there's a way to enforce this in flake8 instead?
  • Didn't hit the Cython code; not exactly sure what the new-style vs. old-style conventions are there.

echo "Check for deprecated messages without sphinx directive DONE"

echo "Check for old-style classes"
grep -R --include="*.py" -E "class\s\S*[^)]:" pandas scripts
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 a regex wizard, so an extra careful look here would be appreciated

@jreback
Copy link
Contributor

jreback commented Mar 30, 2018

maybe we should add some extra flake8 plugins
https://pypi.python.org/pypi?%3Aaction=search&term=flake8&submit=search

would need to add to the requirements_dev and prob modify the ci slightly to install

can u make a separate issue?

@mroeschke
Copy link
Member

There's a Python package called hacking that has flake8 extensions that seems to have a lint check for this:

H238 = hacking.checks.python23:hacking_no_old_style_class

@codecov
Copy link

codecov bot commented Mar 31, 2018

Codecov Report

Merging #20563 into master will not change coverage.
The diff coverage is 95.23%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #20563   +/-   ##
=======================================
  Coverage   91.84%   91.84%           
=======================================
  Files         153      153           
  Lines       49256    49256           
=======================================
  Hits        45241    45241           
  Misses       4015     4015
Flag Coverage Δ
#multiple 90.23% <95.23%> (ø) ⬆️
#single 41.9% <0%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/io/common.py 70.04% <0%> (ø) ⬆️
pandas/io/sas/sas7bdat.py 91.07% <100%> (ø) ⬆️
pandas/io/sas/sas_constants.py 100% <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 5edc5c4...65ccafd. Read the comment docs.

@jreback jreback added the Code Style Code style, linting, code_checks label Mar 31, 2018
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.

lgtm. some renamings. ping on green.



class index:
class index(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename this class (capitalize, but needs a differnt name then)


def test_loadFileLikeObject(self):
class filelike:
class filelike(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename this

return str(type(x))

class fn_class:
class fn_class(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename this

@jschendel jschendel force-pushed the use-new-style-classes branch from 16c8418 to 65ccafd Compare April 3, 2018 03:01
@jschendel
Copy link
Member Author

@jreback : green

@jorisvandenbossche jorisvandenbossche merged commit e71c02a into pandas-dev:master Apr 3, 2018
@jorisvandenbossche
Copy link
Member

@jschendel Thanks!

@jschendel jschendel deleted the use-new-style-classes branch September 24, 2018 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Style Code style, linting, code_checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants