Skip to content

Conversation

@samredai
Copy link
Contributor

@samredai samredai commented Feb 1, 2022

This adds --line-length to the black autoformatter and format checker commands in tox.ini

This may cause some open PRs to require rebasing (there's never a good time to change an auto-formatter) but alternatively we can keep this PR open and wait for a dip in open PRs to rebase this PR, run tox -e format, and commit the updates.

The first commit fcddb35 is the actual change to the formatter, the second commit 66325f4 is the result of running tox -e format on the project.

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

LGTM

@rdblue
Copy link
Contributor

rdblue commented Feb 2, 2022

What was the line length without this? Do we need to do this?

@samredai
Copy link
Contributor Author

samredai commented Feb 2, 2022

@rdblue the default for black is 88. From what I've seen, that's usually bumped up to somewhere in the range of 110-130. To see some of the side-effects of a low line-length setting, see this comment on another PR.

@rdblue rdblue merged commit fc899b3 into apache:master Feb 2, 2022
@rdblue
Copy link
Contributor

rdblue commented Feb 2, 2022

88 seems a bit small to me. Since we should do this when it will introduce as few changes as possible, I'll go ahead and merge. Thanks, @samredai!

@samredai
Copy link
Contributor Author

samredai commented Feb 2, 2022

Thanks @rdblue. I'm going to open a quick follow-up PR to autoformat again. Looks like a PR was merged after I opened this so wasn't caught here!

@rdblue
Copy link
Contributor

rdblue commented Feb 2, 2022

Thanks!

@jun-he
Copy link
Collaborator

jun-he commented Feb 3, 2022

Thanks @samredai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants