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

Enforce linting via the test suite #79

Merged
merged 11 commits into from
Nov 16, 2015
Merged

Enforce linting via the test suite #79

merged 11 commits into from
Nov 16, 2015

Conversation

john-kurkowski
Copy link
Owner

After many contributions, tldextract's feature set has largely solidified. However its code became a patchy mess. Not much left to do, except make tldextract a sterling work of art. 😉

One approach is to make sure the coding style is relatively uniform. So this PR adds the developer requirement of Pylint.

It has wide editor support. It has a nicely restrictive default set of rules, yet can be customized per folder. It detects code complexity. It can be invoked programmatically.

I found no off-the-shelf way to enforce linting across all a Python project's contributors. Local Git hooks have to be installed per person. Can't edit GitHub's remote Git hooks. Editor support has to be installed per person. What's the one thing left, that every contributor's code funnels through? CI.

This PR programmatically generates the tests for every Python lint error in the project. It relaxes some of Pylint's default rules. But it embraces most of them, and fixes the project's violations, making tldextract significantly more uniform and readable.

Limitations

  • Pylint doesn't support Python 2.6 and can't yet run on Python 3.5. Skip the lint check for these versions. Besides, it's the same source code covered by the other Python versions.
  • This only adds lint errors to the test suite if they exist. It is impossible to enumerate the absence of lint errors.

Tasks

  • Update README with new developer requirements
  • Enforce Pylint via tldextract's test suite
  • pylintrc, lightly relaxed
  • Fix lint non-refactor errors
  • Fix lint errors requiring a refactor (see Python 2.7 CI failure)
  • Fix 0 exit code in CI, even when test failures present
  • Skip linting Python versions incompatible with Pylint (2.7, 3.5)
  • Python 3.x compat

References

Most readers are users, not contributors. Only contributors ought to run the test suite.
Run Pylint on every project Python file. Dynamically generate a test case for every Pylint failure. Require Pylint for development via requirements.txt.

References:
* http://blog.jameskyle.org/2014/05/pep8-pylint-tests-with-nose-xunit/
* http://stackoverflow.com/a/20870875/62269
* http://stackoverflow.com/a/33714768/62269
* Relax function name length. I like self-descriptive function names, which can be long, in moderation.
* Relax docstring-min-length. Code that short should be self-documenting. Relatedly, test cases don't need a docstring.
* Relax min-public-methods. I understand the OOP lecture Pylint is trying to give. This project and its collaborators aren't at risk of that.
* Allow GAE constant name
* Allow TODOs. I can grep for those myself just fine.
Remove obsolete HTTP RPC route to run the test suite. Travis CI does that now.
Jumping through hoops for an old version of Python isn't worth it. Pylint 1.4 is superior to Pylint 1.3. It's the same code for all versions. If we have to skip Python 2.6 lint checks, so be it.

This mostly reverts commit 8882913. Keep the Pylint subprocess stderr checking.
Skip linting for Pylint's unsupported Python versions, 2.6 and 3.5
Document _get_tld_extractor's recipe and split it into several private methods.
@john-kurkowski john-kurkowski changed the title [WIP] Enforce linting via the test suite Enforce linting via the test suite Nov 15, 2015
@floer32
Copy link
Collaborator

floer32 commented Nov 16, 2015

Nice work!!!

@john-kurkowski
Copy link
Owner Author

Thank you. I clearly didn't have anything better to do this weekend. 😄

john-kurkowski added a commit that referenced this pull request Nov 16, 2015
Enforce linting via the test suite
@john-kurkowski john-kurkowski merged commit 85b2e09 into master Nov 16, 2015
@john-kurkowski john-kurkowski deleted the pylint branch November 18, 2015 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants