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

SCons: Format buildsystem files with psf/black #37421

Merged
merged 4 commits into from
Mar 30, 2020

Conversation

akien-mga
Copy link
Member

Configured for a max line length of 120 characters:

find \( -name "SConstruct" -o -name "SCsub" -o -name "*.py" \) -exec black -l 120 {} \;

psf/black is very opinionated and purposely doesn't leave much room for
configuration. The output is mostly OK so that should be fine for us,
but some things worth noting:

  • Manually wrapped strings will be reflowed, so by using a line length
    of 120 for the sake of preserving readability for our long command
    calls, it also means that some manually wrapped strings are back on
    the same line and should be manually merged again.

  • Code generators using string concatenation extensively look awful,
    since black puts each operand on a single line. We need to refactor
    these generators to use more pythonic string formatting, for which
    many options are available (%, format or f-strings).

  • CI checks and a pre-commit hook will be added to ensure that future
    buildsystem changes are well-formatted.

@akien-mga
Copy link
Member Author

Review comments welcome to identify locations where the formatted output doesn't look good and could be improved (which means changing the code, as we can't configure the formatter beyond line length).

As I mentioned in the OP, code generators look ugly and should be refactored, but it's a significant amount of busy work so I'm leaving it for a future PR (or a contributor? ;)).

Configured for a max line length of 120 characters.

psf/black is very opinionated and purposely doesn't leave much room for
configuration. The output is mostly OK so that should be fine for us,
but some things worth noting:

- Manually wrapped strings will be reflowed, so by using a line length
  of 120 for the sake of preserving readability for our long command
  calls, it also means that some manually wrapped strings are back on
  the same line and should be manually merged again.

- Code generators using string concatenation extensively look awful,
  since black puts each operand on a single line. We need to refactor
  these generators to use more pythonic string formatting, for which
  many options are available (`%`, `format` or f-strings).

- CI checks and a pre-commit hook will be added to ensure that future
  buildsystem changes are well-formatted.
@akien-mga akien-mga force-pushed the python-format-black branch 3 times, most recently from c8d02a8 to f05d582 Compare March 30, 2020 07:20
Also install and use pygmentize to visualize clang-format and black
diffs.
@akien-mga akien-mga merged commit b383484 into godotengine:master Mar 30, 2020
@Xrayez
Copy link
Contributor

Xrayez commented Mar 30, 2020

As I mentioned in the OP, code generators look ugly and should be refactored, but it's a significant amount of busy work so I'm leaving it for a future PR (or a contributor? ;)).

Worth noting SCons provides Textfile() and Substfile() builders which could likely help this to some extent.

@akien-mga
Copy link
Member Author

Cherry-picked for 3.2.2.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 10, 2020
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.

2 participants