Skip to content

Raise line limit to 99 chars in linter.#2209

Closed
mythmon wants to merge 6 commits intomozilla:masterfrom
mythmon:tweak-linting
Closed

Raise line limit to 99 chars in linter.#2209
mythmon wants to merge 6 commits intomozilla:masterfrom
mythmon:tweak-linting

Conversation

@mythmon
Copy link
Copy Markdown
Contributor

@mythmon mythmon commented Nov 6, 2014

A while ago pep8 introduced a controversial change that allowed for lines up to 99 characters long, instead of the traditional 79, in the case that everyone on the project could agree that they liked 99 better.

I think we can all agree on 99 (if I'm wrong, please speak up). In this PR, I have also convinced the robots to like 99 too. This only applies to Python files.

I also moved the exclude list from the lint script and into the flake8 config file. I think it's better there, even if it looks ugly.

Comment thread setup.cfg Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know configparser supports multiple lines, but I don't know offhand if whatever flake8 uses does. It's worth trying this:

exclude = *migrations*,
    kitsune/settings*,
    kitsune/sumo/db_strings.py,
    kitsune/sumo/static/js/libs/ace/kitchen-sink/docs/python.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That didn't work either. I also tried

exclude = *migrations*, \
    kitsune/settings*, \
    kitsune/sumo/db_strings.py, \
    kitsune/sumo/static/js/libs/ace/kitchen-sink/docs/python.py

before and that didn't work. I wasn't sure what the parser being used was so I had a hard time looking it up. I'll go see if the configparser docs are useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The configparser docs say you're right, and that as long as the second line is indented more than the first, it will be fine. However, it seems pep8/flake8 are supremely picky about the excludes format. I tried simply putting spaces after the commas, and it didn't work.

By the way, I'm defining "didn't work" as throwing lint errors in settings.py. I suspect flake8 (or whatever is parsing this value) isn't stripping whitespace next to commas like I would expect it to, and that when there is a newline in a configparser, it either includes the newline, the indentation spaces, a space out of nowhere, or some combination of all of those.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

flake8 passes it directly to pep8 which doesn't handle it right for exactly the reasons you theorize:

PyCQA/pycodestyle#339

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I threw together a PR: PyCQA/pycodestyle#343

However that project looks somewhat inactive, so I'm not sure when/if that'll get merged. I tried to make it as easy-to-review and complete as possible so as to increase the likelihood it lands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

\o/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking at the pep8 github repo, I think you're right about it not being very active. I'm not hopeful that your patch will get merged any time soon. I see 3 options:

  1. Deal with having crappy really long lines in this config file.
  2. Move this setting back to the bash script where it was fairly readable, but would only apply when using our bash script. Which is probably fine.
  3. Use a different version of pep8 that has your patch, or one like it, applied. I don't really want to become a maintainer for pep8, but maybe someone else does.
  4. Switch to a different linter that is more maintained.

@mythmon
Copy link
Copy Markdown
Contributor Author

mythmon commented Nov 10, 2014

Besides @willkg's (very valid) complaint with the linter setttings, are there any other concerns blocking this?

@mythmon
Copy link
Copy Markdown
Contributor Author

mythmon commented Nov 10, 2014

I worked with @willkg to integrate a new version of pep8 that supports a nicer syntax for the ignore setting. We went on a magical adventure through the lands of Pip and Peep.

Then I learned that the version of pep8 that willkg based his version off of includes a new lint rule, E731, which is about assigning lambda to variables. It does seem weird to make an anonymous function and then de-anonymize it right away. So I went and fixed all those, rather blindly. Some of them are kind of goofy.

@mythmon
Copy link
Copy Markdown
Contributor Author

mythmon commented Nov 11, 2014

That commit should fix the tests.

@mythmon
Copy link
Copy Markdown
Contributor Author

mythmon commented Nov 12, 2014

Ha. There was another problem. I fixed that one too. Travis isn't really working in my browser, so I had trouble finding the errors it was having. Travis is passing now. Can I land this?

@rlr
Copy link
Copy Markdown
Contributor

rlr commented Nov 12, 2014

I'm ok with it!

@mythmon
Copy link
Copy Markdown
Contributor Author

mythmon commented Nov 12, 2014

/me lands it.

@mythmon
Copy link
Copy Markdown
Contributor Author

mythmon commented Nov 12, 2014

92ebbb9 Fix all the new lint errors about lambda.
1bb4f75 Raise line limit to 99 chars in linter.
e04652f Upgrade pep8 to willkg's downstream version.

@mythmon mythmon closed this Nov 12, 2014
@mythmon mythmon deleted the tweak-linting branch November 12, 2014 19:57
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