8000 Python: Follow the PEP 8 style guide · Issue #1338 · auto-pts/auto-pts · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Python: Follow the PEP 8 style guide #1338

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.

Sign up for GitHub

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

Jump to bottom
Open
Thalley opened this issue Jan 10, 2025 · 4 comments
Open

Python: Follow the PEP 8 style guide #1338

Thalley opened this issue Jan 10, 2025 · 4 comments

Comments

@Thalley
Copy link
Collaborator
Thalley commented Jan 10, 2025

https://peps.python.org/pep-0008/ is the official (and probably most used) style guide for Python code. It has a long set of rules to help improve readability and reduce errors.

The autopts codebase should be updated to follow the PEP 8 style guide. To help this process, a linter should be applied. https://docs.astral.sh/ruff/ would be a good suggestion here, due to it's efficiency and flexibility.

Suggest approach:

  1. Add a linter to CI with annotations in code review
  2. Start updating the codebase to follow PEP 8. In the case that this is a major undertaking, suggest to apply the changes on a file-by-file basis, so we don't have a single enormous PR that will conflict with everything, and make it harder to review
  3. Once 2. is done, change the CI action to reject code changes that do not follow PEP 8 to make all code in the future follow PEP 8

We can modify and apply our own rules to the project. A common change is increasing the line length. PEP 8 by default only allows for maximum 79 characters per line: https://peps.python.org/pep-0008/#maximum-line-length and PEP 8 does say it's OK to increase to 99, but in the end it's up to use to decide. Some projects use 100 or 120.
@Thalley Thalley mentioned this issue Jan 10, 2025
Merged
@JakubMarciniak93
Copy link
Contributor
JakubMarciniak93 commented Apr 11, 2025 •
edited
Loading

First part available in PR:
#1440
implement PEP 8 rule checking here using ruff, rules checked:
"F", "E", "W", "I", "UP" except that the F841 notification is ignored due to many variables not used in the project - but there is a risk of using these variables in internal tests of many users.

At the request of @sjanc
TO DO:

  • getting rid of lazy import nested in functions most often in the wid module
  • getting rid of all imports import *, currently there are places like init.py where we use import * which is not correct. We should only use necessary things.
  • improvement of tools structure - there are E402 exceptions thrown in that prevent the imports from being arranged correctly. The order of imports is important for the bot to work correctly. Theoretically, it can be fixed and the order of imports should be consistent with ruff. Example: tools -> active_hub_server.py
AUTOPTS_REPO = dirname(dirname(abspath(__file__)))
sys.path.insert(0, AUTOPTS_REPO)

from autopts.utils import (  # noqa: E402 # the order of import is very important here
    hid_gpio_hub_set_usb_power,
    ykush_set_usb_power,
)

@Thalley
Copy link
Collaborator Author
Thalley commented Apr 11, 2025

There are likely a handful of larger modifications needed to follow PEP8 properly. While each PEP rule are usually small and contained, some of the design choices in Python will conflict with that.

For example regarding the variables not being used, there are some places where we access a variable directly from another module, e.g. the board_type in https://github.com/auto-pts/auto-pts/blob/master/autopts/ptsprojects/boards/nrf53.py#L22

I think that is actually good for us, as we do not want to just follow the PEP8 syntax, we generally we should take this as an opportunity to find design choices (usually old ones) that are not very "pythonic" and refactor those.

@JakubMarciniak93
Copy link
Contributor

For example regarding the variables not being used, there are some places where we access a variable directly from another module, e.g. the board_type in https://github.com/auto-pts/auto-pts/blob/master/autopts/ptsprojects/boards/nrf53.py#L22

I feel tempted to put such variables in "common" files. So that it would be easy to manipulate them collectively if necessary. Despite everything, these are still suggestions.

@Thalley
Copy link
Collaborator Author
Thalley commented Apr 11, 2025

For example regarding the variables not being used, there are some places where we access a variable directly from another module, e.g. the board_type in https://github.com/auto-pts/auto-pts/blob/master/autopts/ptsprojects/boards/nrf53.py#L22

I feel tempted to put such variables in "common" files. So that it would be easy to manipulate them collectively if necessary. Despite everything, these are still suggestions.

Depends on the situation. In many cases (including the aforementioned board files), I think we would benefit significantly from defining a class and then get/set the values via that instead, i.e. start becoming more object oriented in our designs rather than the current approach that is very imperative / C-like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Thalley @JakubMarciniak93
0