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.

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

Add new DISABLE_SPLIT_LIST_WITH_COMMENT flag. #1177

Merged
merged 4 commits into from
Nov 8, 2023
Merged

Add new DISABLE_SPLIT_LIST_WITH_COMMENT flag. #1177

merged 4 commits into from
Nov 8, 2023

Conversation

jlebar
Copy link
Contributor

@jlebar jlebar commented Nov 2, 2023

DISABLE_SPLIT_LIST_WITH_COMMENT is a new knob that changes the
behavior of splitting a list when a comment is present inside the list.

Before, we split a list containing a comment just like we split a list
containing a trailing comma: Each element goes on its own line (unless
DISABLE_ENDING_COMMA_HEURISTIC is true).

Now, if DISABLE_SPLIT_LIST_WITH_COMMENT is true, we do not split every
element of the list onto a new line just because there's a comment somewhere
in the list.

This mirrors the behavior of clang-format, and is useful for e.g. forming
"logical groups" of elements in a list.

Note: Upgrading will result in a behavioral change if you have
DISABLE_ENDING_COMMA_HEURISTIC in your config. Before this version, this
flag caused us not to split lists with a trailing comma and lists that
contain comments. Now, if you set only that flag, we will split lists
that contain comments. Set the new DISABLE_SPLIT_LIST_WITH_COMMENT flag to
true to preserve the old behavior.

@jlebar
Copy link
Contributor Author

jlebar commented Nov 2, 2023

cc @ptillet

@jlebar jlebar marked this pull request as ready for review November 2, 2023 21:55
@jlebar
Add new DISABLE_SPLIT_LIST_WITH_COMMENT flag.
bf301f5 
`DISABLE_SPLIT_LIST_WITH_COMMENT` is a new knob that changes the
behavior of splitting a list when a comment is present inside the list.

Before, we split a list containing a comment just like we split a list
containing a trailing comma: Each element goes on its own line (unless
`DISABLE_ENDING_COMMA_HEURISTIC` is true).

Now, if `DISABLE_SPLIT_LIST_WITH_COMMENT` is true, we do not split every
element of the list onto a new line just because there's a comment somewhere
in the list.

This mirrors the behavior of clang-format, and is useful for e.g. forming
"logical groups" of elements in a list.

Note: Upgrading will result in a behavioral change if you have
`DISABLE_ENDING_COMMA_HEURISTIC` in your config.  Before this version, this
flag caused us not to split lists with a trailing comma *and* lists that
contain comments.  Now, if you set only that flag, we *will* split lists
that contain comments.  Set the new `DISABLE_SPLIT_LIST_WITH_COMMENT` flag to
true to preserve the old behavior.
@jlebar jlebar mentioned this pull request Nov 3, 2023
Merged
jlebar added a commit to triton-lang/triton that referenced this pull request Nov 3, 2023
@jlebar
Reformat Python code with yapf.
e25ebb6 
I've add an option to yapf to do what we want for long lines, see
google/yapf#1177.  We can now have a real Python
formatter, yay!

To make this PR, I ran my modified yapf over the repository, then looked
over the full diff.  Where yapf was mangling the param list of long
function decls/calls (mostly kernels), I manually added `#` to put
linebreaks where we want.  I fixed up other formatting too -- mostly
adding or removing a trailing comma from lists.

Overall, trailing `#` was sufficient to get formatting similar to our
current code.  I didn't have to disable yapf anywhere.
jlebar added a commit to triton-lang/triton that referenced this pull request Nov 3, 2023
@jlebar
Reformat Python code with yapf.
7e27146 
I've add an option to yapf to do what we want for long lines, see
google/yapf#1177.  We can now have a real Python
formatter, yay!

To make this PR, I ran my modified yapf over the repository, then looked
over the full diff.  Where yapf was mangling the param list of long
function decls/calls (mostly kernels), I manually added `#` to put
linebreaks where we want.  I fixed up other formatting too -- mostly
adding or removing a trailing comma from lists.

Overall, trailing `#` was sufficient to get formatting similar to our
current code.  I didn't have to disable yapf anywhere.
ptillet added a commit to triton-lang/triton that referenced this pull request Nov 3, 2023
@jlebar @ptillet
Reformat Python code with yapf. (#2589)
df08301 
I've add an option to yapf to do what we want for long lines, see
google/yapf#1177.  We can now have a real Python
formatter, yay!

To make this PR, I ran my modified yapf over the repository, then looked
over the full diff.  Where yapf was mangling the param list of long
function decls/calls (mostly kernels), I manually added `#` to put
linebreaks where we want.  I fixed up other formatting too -- mostly
adding or removing a trailing comma from lists.

Overall, trailing `#` was sufficient to get formatting similar to our
current code.  I didn't have to disable yapf anywhere.

---------

Co-authored-by: Phil Tillet <[email protected]>
bwendling
bwendling reviewed Nov 6, 2023
View reviewed changes
CHANGELOG.md Outdated
`DISABLE_ENDING_COMMA_HEURISTIC` in your config. Before this version, this
flag caused us not to split lists with a trailing comma *and* lists that
contain comments. Now, if you set only that flag, we *will* split lists
that contain comments. Set the new `DISABLE_SPLIT_LIST_WITH_COMMENT` flag to
Copy link
Member

Choose a reason for hiding this comment

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

This confuses me. You say that with this flag:

we will split lists that contain comments

But isn't that exactly what this flag is supposed to prevent?

Copy link
Contributor Author

@jlebar jlebar Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's right?

You say that with this flag:

we will split lists that contain comments

That's not what I intended to convey. In the context above, "this flag" refers to DISABLE_ENDING_COMMA_HEURISTIC.

To be more clear, here's what changes.

  • Before:

    • Default: split lists with a trailing comma or with comments.
    • DISABLE_ENDING_COMMA_HEURISTIC=true, do not split lists with a trailing comma or with comments.

  • After:

    • Default: Same as before.
    • DISABLE_ENDING_COMMA_HEURISTIC=true and DISABLE_SPLIT_LIST_WITH_COMMENT=false, do not split lists with trailing comma, but do split lists with comments. This is different than before.
    • DISABLE_ENDING_COMMA_HEURISTIC=false and DISABLE_SPLIT_LIST_WITH_COMMENT=true, do split lists with a trailing comma, but do not split lists with comments. You used to get this behavior just by setting DISABLE_ENDING_COMMA_HEURISTIC=true, but now you need to set two flags.
    • Both flags true: do not split lists with a trailing comma, and do not split lists with comments.

(I hope I got that right above.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Okay.

Could you add the interaction with DISABLE_ENDING_COMMA_HEURISTIC in the README.md and style.py descriptions? I want to make sure people know about any any unforeseen side-effects. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please have a look. And thanks again!

@jlebar
Copy link
Contributor Author

jlebar commented Nov 6, 2023

Thank you for the review!

@jlebar
Update docs.
cc2b6a7 
Write more in CHANGELOG.md, and add pointers to the changelog in
README.md and style.py.
bwendling
bwendling approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@bwendling bwendling 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 excellent work. And also for adding documentation for DISABLE_SPLIT_LIST_WITH_COMMENT. :-)

@bwendling bwendling merged commit f36b8c2 into google:main Nov 8, 2023
11 checks passed
@jlebar
Copy link
Contributor Author

jlebar commented Nov 8, 2023

Woo, thank you for merging!

@jlebar jlebar deleted the comment-in-long-arg-list branch November 8, 2023 21:49
@jlebar jlebar restored the comment-in-long-arg-list branch November 10, 2023 18:34
@jlebar
Copy link
Contributor Author

jlebar commented Nov 10, 2023

Note to self: This branch is referred to in the Triton pre-commit, so I should not delete it. :)

pingzhuu pushed a commit to siliconflow/triton that referenced this pull request Apr 2, 2024
@jlebar @ptillet
Reformat Python code with yapf. (triton-lang#2589)
0922ae1 
I've add an option to yapf to do what we want for long lines, see
google/yapf#1177.  We can now have a real Python
formatter, yay!

To make this PR, I ran my modified yapf over the repository, then looked
over the full diff.  Where yapf was mangling the param list of long
function decls/calls (mostly kernels), I manually added `#` to put
linebreaks where we want.  I fixed up other formatting too -- mostly
adding or removing a trailing comma from lists.

Overall, trailing `#` was sufficient to get formatting similar to our
current code.  I didn't have to disable yapf anywhere.

---------

Co-authored-by: Phil Tillet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwendling bwendling bwendling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jlebar @bwendling