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

add pyproject.toml #1800

Merged
merged 3 commits into from
Aug 23, 2021
Merged

add pyproject.toml #1800

merged 3 commits into from
Aug 23, 2021

Conversation

umarcor
Copy link
Member

@umarcor umarcor commented Jun 19, 2021

This PR adds a pyproject.toml file for specifying the line-length used by black, which is increased from 88 to 120.

@Xiretza
Copy link
Contributor

Xiretza commented Jun 20, 2021

it's obviously completely subjective, but personally I find 120 to be too long since it causes too many wrapped lines in my editor - I usually go with 100.

@umarcor
Copy link
Member Author

umarcor commented Jun 20, 2021

This is going to be fun 🤣.

  • Tristan uses 80 chars in Ada sources, because that's the size of his punched card reader (sic).
  • Black uses 88 by default because of not very clear reasons: Change default line length to 100  psf/black#290.
  • You suggested 100.
  • I use 120 in the docs, and that's also the default in my editor.
  • Patrick uses 160, at least for Python.

I find 80/88 to be too narrow and >125 to be too wide. Hence, I'm good with any value between 100 and 120.

Nonetheless, I wanted to use this issue for discussion too. We should be able to document how each contributor can use a different value locally, and use hooks or some other helper script for converting to/from their desired value. There is CLI option -l, which allows overriding the value in the configuration (or the default):

$ black pyGHDL -l 100

Very unfortunately, I found that black is not deterministic... Try the following:

$ black pyGHDL -l 50
$ black pyGHDL -l 120
$ black pyGHDL -l 100
$ black pyGHDL

I would expect no changes after the last call, since that's the default in the repo. But you will see that some of the modifications done in -l 50 are preserved untouched in further runs. Maybe black is not the best formatter for this purpose?

"Caught exception while handling {}, "
+ "see VHDL language server output for details."
).format(method),
("Caught exception while handling {}, " + "see VHDL language server output for details.").format(
Copy link
Contributor

@Xiretza Xiretza Jun 20, 2021

Choose a reason for hiding this comment

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

Regardless of line length, the + here is unnecessary. Weird that black doesn't catch something like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

See 11f05af. It seems that black does not break those long strings, so the 88 char width is not respected. I guess @tgingold might have used + to work around that.

@Paebbels
Copy link
Member

As @umarcor mentioned I personally use longer lines in my personal projects.
But I'll stick to what we decide for pyGHDL as it is enforced by black.

For pyGHDL, I did a test with black (default = unreasonable 88 chars) with 120 chars and 160 chars.

Against 80/88 chars:

  • many lines get broken by black into lists.
    E.g. some files have more then 1 monitor spage of import statements before code starts

For is for 120 chars:

  • Huge improvement for the number of lines, so black generates less lists or deeply nested parenthesis structures
  • My diff tool displays inclusive sidebars 2x 120 chars side-by-side (24" display with 2560x1600 px)

Against 160 chars:

  • Most people would complain about it and my request to use a state of the art dual monitor setup or just a single 49" monitor in 32:10 aspect ratio to handle side-by-side views would be seen as ... :)

I don't want to use short variable names and ambivalent abbreviations so black can format my code in a compact form.

Another idea for avoiding long lines is to change the indentation for 4 to 2 spaces like it's used by JSON, YAML, XML, ...


So personally, I don't like black as it makes us all slaves of a tool not supporting code coding styles for readability and efficiency, but rather a fixed pattern so programmers are force the workaround the rules of black.

@Xiretza
Copy link
Contributor

Xiretza commented Jun 20, 2021

Another idea for avoiding long lines is to change the indentation for 4 to 2 spaces like it's used by JSON, YAML, XML, ...

That would go directly against PEP 8, which is the one standard almost everyone agrees should be a base for any python formatting. Then again, it also mandates 80 character lines...

The ideal solution for this would of course be tabs, which everyone can set to whatever width they want, but that doesn't work for python (because you wouldn't be able to use spaces for alignment).

@umarcor
Copy link
Member Author

umarcor commented Jun 20, 2021

I don't want to use short variable names and ambivalent abbreviations so black can format my code in a compact form.

Agree.

So personally, I don't like black as it makes us all slaves of a tool not supporting code coding styles for readability and efficiency, but rather a fixed pattern so programmers are force the workaround the rules of black.

I would be good with any other alternative, such as yapf (see https://www.kevinpeters.net/auto-formatters-for-python). However, someone needs to go through the options of those non-opinionated formatters and select some defaults. If they can provide deterministic output for varying contributor configurations, I believe the change is sensible.

Personally, I don't mind dealing with the annoyances of black, as I wouldn't mind dealing with the ones from any other formatter or style preference. What I cannot and don't want to do is to learn coding styles myself.

@Paebbels
Copy link
Member

@tgingold what do you think about 120 chars compared to 88 chars default from back.

Currently black creates such ugly code:

        return cls(
            variableNode,
            [
                name,
            ],
            subtypeIndication,
        )

@tgingold
Copy link
Member

tgingold commented Aug 23, 2021 via email

@umarcor
Copy link
Member Author

umarcor commented Aug 23, 2021

I rebased and addressed the usage of + (as suggested by @Xiretza).

I propose we merge this (black with 120 char setting), until we find an alternative formatter which generates deterministic output.

@Paebbels Paebbels merged commit c58dff1 into ghdl:master Aug 23, 2021
@umarcor umarcor added this to the v2.0 milestone Aug 24, 2021
@umarcor umarcor deleted the py-width branch August 24, 2021 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants