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 style and formating files for Python. Format Python code. #3870

Merged
merged 14 commits into from
Apr 11, 2023

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jan 31, 2023

@vlstill @davidbolvansky

Adding a .pylintrc and a pytoml for style recommendations and formatting. Line-width is 100 because the majority of the files in this repository are written for a width of 100 characters. Yapf was chosen as formatter because it is configurable, as opposed to black, which enforces a particular style. Black is the formatter of choice because it is well maintained, by official Python maintainers, and requires little configuration.

.pylintrc Outdated

# Minimum Python version to use for version dependent checks. Will default to
# the version used to run pylint.
py-version=3.10
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be specified? It seems like something that will be hard to find once the build images are updated and newer minimal python is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can probably set it to 3.8 instead, the standard Ubuntu 20.04 version.

@fruffy fruffy marked this pull request as ready for review February 16, 2023 19:31
@fruffy
Copy link
Collaborator Author

fruffy commented Feb 16, 2023

@vlstill Any other comments?

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Thank you for this, having a consistent formatting is definitely step in the right direction and I think the formatting is generally good. However, I can see the following problems (all also marked in code):

  • The comments inside multi-line lists are pushed right beyond the longest list element even if they are on otherwise-empty lines. This looks wrong and makes these comments hard to read.
  • Formatting of long calls spreads the code over too many lines (especially if the arguments are short), but it might still be worth the consistency.
  • The formatting of infix operator chains is suboptimal, but I'm afraid there is no good way to do this in Python.

backends/ebpf/tests/ptf/checksum.py Outdated Show resolved Hide resolved
tools/cpplint.py Outdated Show resolved Hide resolved
tools/cpplint.py Outdated Show resolved Hide resolved
tools/driver/p4c_src/main.py Outdated Show resolved Hide resolved
backends/ebpf/tests/ptf/common.py Outdated Show resolved Hide resolved
Comment on lines 63 to 64
exp_pkt = Ether(dst="11:11:11:11:11:11") / MPLS(
label=20, cos=5, s=1, ttl=64) / testutils.simple_ip_only_packet(ip_dst="192.168.1.1")
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is the most readable due to the header name being split from its content, but I don't see a good alternative, as both

        exp_pkt = Ether(dst="11:11:11:11:11:11") / \
            MPLS(label=20, cos=5, s=1, ttl=64) / \
            testutils.simple_ip_only_packet(ip_dst="192.168.1.1")

and

        exp_pkt = (Ether(dst="11:11:11:11:11:11") /
            MPLS(label=20, cos=5, s=1, ttl=64) /
            testutils.simple_ip_only_packet(ip_dst="192.168.1.1"))

look somewhat weird. For me, the last one might be acceptable and so is the one you have chosen. I'd probably avoid the \ version due to weird combination with /.

@fruffy
Copy link
Collaborator Author

fruffy commented Feb 27, 2023

Switching to black, which has a very uncompromising code style. Black is maintained by Łukasz Langa, who maintains the Python packages. This should give us a rather canonical Python style.

The reason I previously chose not to use that was because of the multi-line inline indentation alignment. However, since we have abandoned that concept, we should be able to use black without issues.

@fruffy fruffy force-pushed the fruffy/python_linting branch 3 times, most recently from b9e43b0 to 2059d40 Compare March 3, 2023 13:44
@fruffy fruffy requested a review from vlstill March 3, 2023 19:45
@fruffy
Copy link
Collaborator Author

fruffy commented Mar 28, 2023

@vlstill Mind giving this another review? I switched to black with some tweaks and the formatting seems sound.

@qobilidop
Copy link
Member

I happen to see this PR while working on updating one of the Python scripts, and have one minor suggestion. I wonder if isort could be used together with black to sort imports besides formatting? isort integrates with black easily so this should be straightforward.

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 10, 2023

I happen to see this PR while working on updating one of the Python scripts, and have one minor suggestion. I wonder if isort could be used together with black to sort imports besides formatting? isort integrates with black easily so this should be straightforward.

Done.

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 11, 2023

@mbudiu-vmw Mind giving this a review?

Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I don't really see any improvement in the readability of the Python code.
I am not sure what these tools bring. If they found some bugs I would be more convinced.

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 11, 2023

I don't really see any improvement in the readability of the Python code. I am not sure what these tools bring. If they found some bugs I would be more convinced.

This PR came up because of a request. These are the benefits I see:

  1. The configurations essentially enforce the code preferences described in human prose in guides like this one.
  2. Applying autoformatting becomes easier because, once the style is enforced across the entire project, one does not have to worry about using a formatter when writing code. With that you do not produce diffs that are unnecessary. This also means you do not have to deal with manual code formatting. The configuration also makes sure everyone uses the same formatter for this particular project.
  3. PR reviews become easier because the linters take care of arguments around style.
  4. Code style is consistent across the entire project, which does help readability (even though what can be considered readable code may be up to debate).

We could also try to apply linters that find bugs. For example, mypy likely would find many inconsistencies in our Python code. But instrumenting those is laborious and the linters are more opinionated.

@fruffy fruffy merged commit 8947d5e into main Apr 11, 2023
@fruffy fruffy deleted the fruffy/python_linting branch April 11, 2023 20:16
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.

4 participants