getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
214 stars 48 forks source link

Could we get rid of the strict line length check? #467

Closed jasperges closed 4 years ago

jasperges commented 4 years ago

I completely agree with trying to be PEP8 compliant and using style checkers. But I don't think it's always good to be so strict with the line width. I feel in certain cases it's better to have lines that are a bit longer then 80 characters, because they can greatly improve readability. See also this great talk by Raymond Hettinger.

See this code from Avalon as an example:

self._messagebox.setStandardButtons(
    self._messagebox.Yes | self._messagebox.No |
    self._messagebox.Cancel
)

I quickly broke those lines because self._messagebox.Yes | self._messagebox.No | self._messagebox.Cancel was 1 character too long. But now readability suffers IMHO. You could argue of course that it would be even better to firstly assign shorter names to the buttons, so that everything could even fit on 1 line (e.g.: self._messagebox.setStandardButtons(yes | no | cancel), but I hope you get the point.

So in short: although I think it's a very good thing to strive for a maximum line width of 80, sometimes it is better to break this rule in my opinion.

Curious to find out what other people think.

tokejepsen commented 4 years ago

Personally I like the strict 80 character rule. I work on a laptop, so being able to half screen code without having to scroll sideways is great!

But also what would be the line limit if not 80? No line limit?

But now readability suffers IMHO.

I dont think it suffers, but I personally have broken all the lines instead of just one.

self._messagebox.setStandardButtons(
    self._messagebox.Yes |
    self._messagebox.No |
    self._messagebox.Cancel
)
jasperges commented 4 years ago

But also what would be the line limit if not 80? No line limit? Most of the time I use 100. But that is of course fairly arbitrary and a personal choice...

I dont think it suffers, but I personally have broken all the lines instead of just one.

self._messagebox.setStandardButtons(
    self._messagebox.Yes |
    self._messagebox.No |
    self._messagebox.Cancel
)

I agree that's better. :)

Let's see if I can find a better example... Now that I think of it, my gripe could very well be with the automatic style checkers and how they 'fix' long lines. A lot of the times, it's better if you do it yourself in my experience.

BigRoy commented 4 years ago

In this case I do agree with Toke that if you allow cases to ignore the rule then you might run into discussions about when it is allowed or not, each time. As long as there's a rule that we can consider a decent standard I'm fine with it.

There have been times I struggled with 80 and felt it was too short, and there have also been times I was very happy that code was consistent with the line length... like when having a part of the screen available for the code.

jasperges commented 4 years ago

Okay. I'm fine with that. You have convinced me. Mainly because with so many people involved it's hard to determine when it should be allowed, as you mention.

On a related note: how would you feel about including a .pylintrc, .style.yapf, maybe isort.cfg and for python 3 even mypy.ini (maybe even more?) so style checking and formatting is configured for this project.

tokejepsen commented 4 years ago

On a related note: how would you feel about including a .pylintrc, .style.yapf, maybe isort.cfg and for python 3 even mypy.ini (maybe even more?) so style checking and formatting is configured for this project.

I'm not familiar with these. What do they do or having for example flake8 linting?

jasperges commented 4 years ago

I think flake8 and pylint are fairly similar. I just happen to use pylint. The .style.yapf is used by yapf a nice tool to quickly format your code (I believe Visual Studio Code also uses this in the background). And isort is used for sorting your imports. And mypy is a static type checker, but is only relevant for Python 3. Anyway, the point is not the specific tools, but to put config files for at least some of these tools in the repo. Then when you are working on the project, your linter/formatter (probably triggered from your favourite code editor somehow) should pick up this configuration. That should help in getting things consistent.

tokejepsen commented 4 years ago

Ahh, right, so it works sort of similar to an auto formatter?

flaixman commented 4 years ago

autopep8 is another auto formatter that works good too if you want to check it :)

jasperges commented 4 years ago

Closing this. The consensus is to stick with a max line width of 80.