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

Define the python linter #42

Open
karlcow opened this issue Jul 9, 2018 · 9 comments
Open

Define the python linter #42

karlcow opened this issue Jul 9, 2018 · 9 comments

Comments

@karlcow
Copy link
Collaborator

@karlcow karlcow commented Jul 9, 2018

There are many different linters for python.
We should try to adopt one style and be using the same. Let's document that in the project.

flake8 is a wrapper around linters. so we can add whichever we think is interesting.

For example pylint would detect the non documented class (missing docstrings).

@karlcow

This comment has been minimized.

Copy link
Collaborator Author

@karlcow karlcow commented Jul 9, 2018

This is related to #41 too.

@karlcow

This comment has been minimized.

Copy link
Collaborator Author

@karlcow karlcow commented Jul 9, 2018

@magsout

This comment has been minimized.

Copy link
Member

@magsout magsout commented Jul 9, 2018

@karlcow

This comment has been minimized.

Copy link
Collaborator Author

@karlcow karlcow commented Nov 5, 2018

  • flake8 for local devs.
  • black if we want to switch to automatic formatting. I'm usually not a big fan of autoformatter, but I can understand how it removes a burden for some people. So I'm open.

@laghee What you do think want?

My current install of flake8 is

Installed plugins: mccabe: 0.6.1, pycodestyle: 2.3.1, pyflakes: 1.6.0
@laghee

This comment has been minimized.

Copy link
Collaborator

@laghee laghee commented Nov 6, 2018

@karlcow I think I have a pretty vanilla version of flake8, and I've been using autopep8 to format (which I kind of hate because it won't automatically break long lines). I don't always love auto-formatting either, but I definitely like to have the option to clean up simple things that are a pain to go back and fix by hand.

I'll try out black for a while and see how that goes.

@laghee

This comment has been minimized.

Copy link
Collaborator

@laghee laghee commented Sep 17, 2019

So, a few things about black defaults as pertains to our current codebase:

(*These 2 defaults can be altered/eliminated with command line options.)

Are any (or all 😁 ) of these going to make you want to rip all your hair out, @karlcow?

My fondness for auto-formatting is mostly about line breaks -- constantly looking up how to correctly split an overly long line in various expressions drives me nuts. But I (or whoever) could always run black locally for that sort of thing.

I think it's worth adding blackas a defined project tool if we're happy with their defaults as is. Or if we're only altering one requirement (line length OR double quotes, but not both). Otherwise it seems like it would add more burden than it removes.

Thoughts?

@karlcow

This comment has been minimized.

Copy link
Collaborator Author

@karlcow karlcow commented Sep 19, 2019

My fondness for auto-formatting is mostly about line breaks -- constantly looking up how to correctly split an overly long line in various expressions drives me nuts.

I think this is the important thing. We don't want you to go to the nuts hospital. 🃏
Specifically if it makes it easier for both people to write code and reviewer to do less nitpicking it would be a good thing.

And that would make a good testbed for the webcompat.com project.

Let's do it, and I'll let you choose the right amount of custom settings or defaults we should use.

@laghee

This comment has been minimized.

Copy link
Collaborator

@laghee laghee commented Nov 23, 2019

I'm going with black configured to leave line length at the library standard of 79 char, but letting it change all quotes to doubles. I'm drafting documentation for dev setup, so I don't want to move on adding it to requirements.txt until I've worked out the configuration setup with python-dotenv, etc.

I'm thinking we should keep flake8 for CircleCI testing for a while to avoid having current tests break (to avoid this we'd have to do a big overhaul for the single-to-double-quote change). As files are touched, we could gradually transition to double quotes.

@karlcow

This comment has been minimized.

Copy link
Collaborator Author

@karlcow karlcow commented Nov 24, 2019

This sounds like a very good plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.