Skip to content

Tweak stickler config: ignore Python files in the docs & disable fixer #1936

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

Merged
merged 14 commits into from
Feb 25, 2018

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Feb 23, 2018

It doesn't always make sense to lint these files fully.

@@ -15,6 +15,11 @@
import xarray as xr
import matplotlib.pyplot as plt


def badly(spaced):
1 + 1/2
Copy link
Contributor

Choose a reason for hiding this comment

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

W1619 division w/o future statement

@@ -15,6 +15,11 @@
import xarray as xr
import matplotlib.pyplot as plt


def badly(spaced):
1 + 1 / 2
Copy link
Contributor

Choose a reason for hiding this comment

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

W1619 division w/o future statement

@fmaussion
Copy link
Member

Maybe we should turn off the fixer also?

@shoyer
Copy link
Member Author

shoyer commented Feb 25, 2018

I would like to disable the fixer (which appears to use autopep8) entirely, for several reasons:

  1. It enforces a very strict style automatically, e.g., removing spaces around arithmetic operators. This actually goes beyond PEP8, which suggests "Use your own judgment". I would rather have comments that I can tell a contributor to ignore and easily disable in the future (via the flake8 config).
  2. The need to pull changes to a pull-request branch from GitHub is a surprising and somewhat disruptive.
  3. I can't figure out how to disable the fixer from only some files.

@shoyer
Copy link
Member Author

shoyer commented Feb 25, 2018

Maybe we should turn off the fixer also?

I'm not sure if you were suggesting turning it off entirely or only for the docs... but as I wrote above, I don't like it either.

@shoyer shoyer force-pushed the stickler-ignore-docs branch from 16ebdf6 to 6150fd9 Compare February 25, 2018 06:23
@fmaussion
Copy link
Member

I would like to disable the fixer (which appears to use autopep8) entirely

Yes, totally, stickler should NOT try to fix anything.

Would you like to turn off stickler entirely or just the auto-fixer? I'm fine with both: while I thinks it's nice to have a tool pointing out pep8 errors (I don't like to do it as a reviewer, it makes me feel like nit-picking), I also find it quite noisy and not very encouraging to submit PRs at an early "quick n dirty" stage, where you really shouldn't care about pep8...

@shoyer shoyer changed the title Tweak stickler config to ignore Python files in the docs. Tweak stickler config: ignore Python files in the docs & disable fixer Feb 25, 2018
@shoyer shoyer merged commit f053567 into pydata:master Feb 25, 2018
@shoyer
Copy link
Member Author

shoyer commented Feb 25, 2018

I also find it quite noisy and not very encouraging to submit PRs at an early "quick n dirty" stage, where you really shouldn't care about pep8...

I agree, but the alternative is to get a failed build error, and it can be a pain to dig through the Travis-CI to find out what went wrong. We use automated comments like this at work to enforce style, and on the whole I think they're probably a good thing.

I would certainly encourage contributors not to worry about these errors until they think their code is ready to submit.

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.

3 participants