Use black for formatting

[umarcor]

This PR uses psf/black: the uncompromising Python code formatter, for formatting Python sources. The line-length is set to 120 characters.

A CI job is added for checking that future contributions are formatted accordingly.

[imphil]

Thanks for this pull request. I'm fine with using black for formatting, we did the same thing already in FuseSoC (but we'll need to get buy-in from @olofk as well). But I have several comments.

• Is there a reason to configure the line length to 120 characters? I'd prefer keeping the black defaults (after all, that's the whole selling point of black). And it is the line length we use in FuseSoC, which is closely enough related to use the same coding style.
• In FuseSoC we have had good experience with using pre-commit to hook up black. Can you copy the setup from there?
• Can you also add a bit of documentation for developers to tell them how to run black when contributing to edalize? (https://fusesoc.readthedocs.io/en/stable/dev/devsetup.html#formatting-and-linting-code for the FuseSoC docs)
• I'm unsure when a good point to merge this PR would be, since it causes merge conflicts on almost all in-flight PRs. We could at least get the setup in at any time, but reformatting everything and enforcing it will need a bit of thought. Maybe we can try to close as many open PRs before, I think I have a bit of capacity in the coming weeks.

And please remember to describe why you're making changes in the commit messages and PR descriptions. For example, it's obvious from the commit itself that you set the line length to 120 characters. But nowhere is a description why that change is made.

[umarcor]

Is there a reason to configure the line length to 120 characters? I'd prefer keeping the black defaults (after all, that's the whole selling point of black). And it is the line length we use in FuseSoC, which is closely enough related to use the same coding style.

The main motivation for using black instead of any other Python formatter is precisely that it is opinionated, so there is almost no room for discusson. That's why we use it in other projects, such as VUnit or GHDL. However, black comes with it's own caveats.

The main problem with using black is that the output is not deterministic. Changing the line-width from x to y and then back to x will not produce the same result as the initial code. That prevents contributors from using a different local setup that the upstream.

At the same time, the default line-length (88) was set arbitrarily (without research), and it is kept mainly for historic/compatibility reasons.

Since we haven't used punch cards for several decades, I believe we can use a line-length which better matches the devices we use for editing in the XXI century. 100-120 seems sensible, as it allows to have two files side-by-side in a 1920 pixel wide screen.

In GHDL and VUnit, we found that using 120 instead of 88 made the code more readable. I think that 100, 110 or 115 might we equally acceptable. I don't have any strong preference above 95.

And please remember to describe why you're making changes in the commit messages and PR descriptions. For example, it's obvious from the commit itself that you set the line length to 120 characters. But nowhere is a description why that change is made.

In this case, it's arbitrary. So, I'm good with you deciding which length to use, or whether to stick with 88.

In FuseSoC we have had good experience with using pre-commit to hook up black. Can you copy the setup from there?

Do you mean adding a .pre-commit-config.yaml file including https://github.com/olofk/fusesoc/blob/master/.pre-commit-config.yaml#L32-L35 ?

Can you also add a bit of documentation for developers to tell them how to run black when contributing to edalize?

There is no "Developer's Guide" section in Edalize's docs. Shall I:

• Create it
• Move "Testing edalize" a level below.
• Add another section and copy https://fusesoc.readthedocs.io/en/stable/dev/devsetup.html#formatting-and-linting-code

?

I'm unsure when a good point to merge this PR would be, since it causes merge conflicts on almost all in-flight PRs. We could at least get the setup in at any time, but reformatting everything and enforcing it will need a bit of thought. Maybe we can try to close as many open PRs before, I think I have a bit of capacity in the coming weeks.

It seems that most PRs except #271 were not updated in a while, so they might be hit by several conflicts regardless of using black. Moreover, they can rebase on top of the PyProject commit, then run black on their branch, last merge. That will reduce the conflicts to the legit ones.

Anyway, I'm good with keeping this PR on hold for some weeks/months, until a better time for introducing these changes.

[imphil]

So, I'm good with you deciding which length to use, or whether to stick with 88.

Let's just keep the default. It's what FuseSoC uses (which is more closely related to edalize than vunit or GHDL), and it's a good example of the "no discussion, just the default" rule.

Do you mean adding a .pre-commit-config.yaml file including olofk/fusesoc@master/.pre-commit-config.yaml#L32-L35 ?

Yes. You can also find the related CI config in FuseSoC.

On documentation: whatever you find most appropriate to point developers to. I certainly wouldn't mind better developer docs in edalize, similar to what we have in FuseSoC (or even better!) But that's clearly beyond the scope of this PR, so two or three lines in https://github.com/olofk/edalize/blob/master/tests/README.rst or the main README would probably be the minimum.

[umarcor]

@imphil, I removed the black settings and run it with the default line-length. I added dev-requirements.txt and .pre-commit-config.yaml. Then, I applied some enhancements to the docs.

[imphil]

Thanks, we're getting there!

In addition to the comments in the code I'd encourage you to structure your commits in a way that every commit is a meaningful, standalone unit of work, and less of a micro-commit where the commit message roughly matches what you typed in the code. When reviewing code, I want to look at a commit, read the commit message to understand why a change was made, and then see the related changes in the code. If a PR has 11 commits which are all effectively mechanical (e.g. "move file from A to B"), the big picture gets lost and reviews get harder.

How about a series of commits like this?

• Restructure documentation to be easier to navigate
• Use black through pre-commit, and document it
• Reformat the whole source tree with black
• Enforce pre-commit in CI

[umarcor]

Restructure documentation to be easier to navigate

Do you approve the six commits that belong to this task? Please, confirm and I will squash. Having smaller commits is defensive, in case the maintainer (you here) wants to remove one of them (such as the line-length setting).

Enforce pre-commit in CI

I don't understand the purpose of the additional Lint workflow. Is that expected to just check (and fail)? Or does it run black and commit the result? In the latter, is it to be used in master only?

Moreover, that Action is in maintenance mode. Shall I use the recommended replacement instead?

[imphil]

I don't understand the purpose of the additional Lint workflow. Is that expected to just check (and fail)? Or does it run black and commit the result? In the latter, is it to be used in master only?

You don't need to make it a separate workflow, but use pre-commit to run black instead of running it manually. That's what the code example that I linked to does.

Moreover, that Action is in maintenance mode. Shall I use the recommended replacement instead?

Whatever approach you prefer.

[olofk]

Thanks for working on this. It looks great to me. As @imphil mentioned, it will wreak havoc on all outstanding PRs but I think we might just need to bite the bullet. Let me just quickly go through them to see if there are any particular ones we want to merge first

[olofk]

Ok, done. I merged a few PRs and the rest will have to wait. If you can just rebase and rerun black to get rid of the conflicts, I'm fine with pulling this in now

[olofk]

Sorry. I decided to merge #274 as well

[umarcor]

@olofk, done.

[olofk]

Muchas gracias! Picked and pushed