Skip to content

Incorporate 'flake8' linting via Travis CI#107

Closed
frewsxcv wants to merge 1 commit intobarosl:masterfrom
frewsxcv:flake8
Closed

Incorporate 'flake8' linting via Travis CI#107
frewsxcv wants to merge 1 commit intobarosl:masterfrom
frewsxcv:flake8

Conversation

@frewsxcv
Copy link

No description provided.

@frewsxcv
Copy link
Author

If this PR is desired, Travis will need to be enabled for this repository

.travis.yml Outdated
Copy link
Author

Choose a reason for hiding this comment

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

For now at least, I've disabled warnings related to line lengths being >80 characters

Choose a reason for hiding this comment

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

You could also make it such that the longest line currently is what you set for max-line-length.

@barosl
Copy link
Owner

barosl commented Nov 3, 2015

I have a mixed feeling about PEP 8... I really like to keep things one-line if that makes sense, and somehow cannot help feeling the urge to write something like a*b + c*d. And I hate the 80-width limit. These all are prohibited by PEP 8, and I didn't particularly like that. It is true that, like you did, some warnings can be turned off case by case, but I think that also lowers the advantages of introducing a style enforcement in the first place. It should be applied all-or-nothing, I think.

But at the same time, it is true that this greatly helps multiple people collaborating on the same code base. It also seems to be wanted often, as there was already a PR that suggested the PEP 8 linting. But it was forgotten and silently closed... I felt bad about the author at the time.

@frewsxcv
Copy link
Author

frewsxcv commented Nov 3, 2015

urge to write something like a*b + c*d

This is a bad practice because one should be grouping things visually via parens, otherwise someone could write something like a+b * c+d which is misleading

but I think that also lowers the advantages of introducing a style enforcement in the first place

Not really, no. Flake8 is more than style (e.g. catches unused imports, undefined variables, etc.) which is very important.

It should be applied all-or-nothing, I think.

Why do you feel this way?

@barosl
Copy link
Owner

barosl commented Nov 3, 2015

This is a bad practice because one should be grouping things visually via parens

This is a valid concern, I think.

Flake8 is more than style (e.g. catches unused imports, undefined variables, etc.)

That's interesting! I didn't know it even catches undefined variables. I definitely need to read the manual some day.

Why do you feel this way?

I think using different options means using a slightly different version of PEP 8, leading to multiple style standards out there that are largely compatible, but still slightly incompatible. So if I'm to adhere to PEP 8, I think I should follow all of its rules. The problem is, like I said above, it is really hard for me to like some rules defined in PEP 8. 😢 But, I think this "all-or-nothing" obsession is my mental problem that needs to be addressed some day. Maybe now.

@sigmavirus24
Copy link

So using a configuration file (e.g., tox.ini, setup.cfg, .flake8) and storing the exceptions or modifications can become your style guide. Each OpenStack project has it's own style guide enumerated in their tox.ini files. You can configure Flake8 like this and that means that anyone developing the project and using flake8 will not have to run it like Travis does. They can just run flake8 and it'll just magically work.

As the maintainer of Flake8, I tend to frown on people adding Flake8 to projects without the owner's consent, but the ideology of the tool is to help enforce a Style Guide. That Style Guide can be tweaked, added to, etc. by you the user and we try to be flexible.

If you have questions, I'm happy to help, but I have no strong opinions on whether you start using Flake8 or not. :)

@cgwalters
Copy link

I wholly agree with the 80 column thing. It's crazy. But I'd say flake8/pylint are worth it, let's just go with the config file?

Probably we should land this after merging most of the outstanding PRs so as to not cause lots of merge conflicts.

@frewsxcv
Copy link
Author

Worth mentioning that #134 also includes flake8 integration.

@ashcrow
Copy link

ashcrow commented Mar 18, 2016

I recommend closing this in favor of #134. No conflict in that PR and it includes this as noted.

@ashcrow
Copy link

ashcrow commented Mar 18, 2016

@barosl I understand your all-or-nothing thought. I tend to be a little overzealous on code conventions myself and fall into this bucket sometimes.

It is possible to turn off or modify specific errors via setup.cfg's [flake8] section. I've found many projects that follow pep8 tend to have at least one exception to it. However, if you'd still like to go all-in on it I'm happy to do some of the grunt work getting code into compliance.

@frewsxcv frewsxcv closed this Mar 18, 2016
@frewsxcv
Copy link
Author

@ashcrow @cgwalters: FYI, servo maintains and utilizes their own fork of Homu you can use and submit pull requests to: https://github.com/servo/homu

@cgwalters
Copy link

@frewsxcv Okay, if there's no motion on #122 then I guess we can resubmit our PRs there for now.

alexcrichton pushed a commit to alexcrichton/homu that referenced this pull request Mar 19, 2018
alexcrichton pushed a commit to alexcrichton/homu that referenced this pull request Mar 19, 2018
Fix privileges check

fixes barosl#107

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/homu/108)
<!-- Reviewable:end -->
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.

5 participants