This is an archive of the discontinued LLVM Phabricator instance.

[NFC][Py Reformat] Reformat python files in llvm
ClosedPublic

Authored by thieta on May 15 2023, 2:05 AM.

Details

Summary

This is the first commit in a series that will reformat
all the python files in the LLVM repository.

Reformatting is done with black.

See more information here:

https://discourse.llvm.org/t/rfc-document-and-standardize-python-code-style

Diff Detail

Event Timeline

thieta created this revision.May 15 2023, 2:05 AM
thieta requested review of this revision.May 15 2023, 2:05 AM
JDevlieghere accepted this revision.May 15 2023, 10:58 AM

LGTM. I would include something in the commit to make things easier for downstream users. If there are conflicts, they're probably better off keeping their version (git checkout --ours) and rerunning the same black command. I would also make sure to specify which version of black to use (23) as the output changes between versions and some distros might have older versions (when using their package manager instead of pip).

This revision is now accepted and ready to land.May 15 2023, 10:58 AM
MatzeB accepted this revision.May 15 2023, 11:27 AM

Thanks for pushing this!

LGTM too

Having a tool-enforces style will make reviewing contributions to the python scripts much easier since we don't have to worry about coding style. I wonder if it would make sense to also add a pre-commit (https://pre-commit.com/) config file to make it easy to automate?

jhenderson accepted this revision.May 17 2023, 12:17 AM

I picked a few random files and saw no issues from my point of view in them. LGTM.

LGTM. I would include something in the commit to make things easier for downstream users. If there are conflicts, they're probably better off keeping their version (git checkout --ours) and rerunning the same black command. I would also make sure to specify which version of black to use (23) as the output changes between versions and some distros might have older versions (when using their package manager instead of pip).

This sounds like a good plan to me.

thieta updated this revision to Diff 522949.May 17 2023, 1:45 AM

Rebased and fixed commit message

This revision was landed with ongoing or failed builds.May 17 2023, 1:49 AM
This revision was automatically updated to reflect the committed changes.

There are many lit.local.cfg files in the tree, I suppose some of them need to be reformatted too.

Ah, I forgot to update the summary in this diff so the final commit message doesn't contain my help info :(

If anyone finds this diff and are having problems with the commit because rebase/merge issues here is what I wrote and the rest of the commits will contain:

This is the first commit in a series that will reformat
all the python files in the LLVM repository.

Reformatting is done with `black`.

If you end up having problems merging this commit because you
have made changes to a python file, the best way to handle that
is to run git checkout --ours <yourfile> and then reformat it
with black.

If you run into any problems, post to discourse about it and
we will try to help.

RFC Thread below:

https://discourse.llvm.org/t/rfc-document-and-standardize-python-code-style

There are many lit.local.cfg files in the tree, I suppose some of them need to be reformatted too.

Thanks for spotting that. Will post another diff for that.

kwk added a subscriber: kwk.May 17 2023, 2:41 AM

@thieta can you please add the commit to the .git-blame-ignore-revs file?

@thieta can you please add the commit to the .git-blame-ignore-revs file?

Already did that as soon as I landed it:

https://github.com/llvm/llvm-project/commit/2e18e7bac9c8e11b55a8b4f6c2b63b22b668cc0b

barannikov88 added inline comments.May 17 2023, 1:48 PM
llvm/utils/lit/lit/main.py
97

It appears that the concatenated strings are not really concatenated. Note the " ".

thieta added inline comments.May 17 2023, 1:53 PM
llvm/utils/lit/lit/main.py
97

In Python you can concat strings without the + sign. In this case it's not necessary- but I doubt it will have any impact on the functionality

barannikov88 added inline comments.May 17 2023, 2:04 PM
llvm/utils/lit/lit/main.py
97

Yeah, I know. This just strains a reader's brain. Might be worth doing a post-processing pass to eliminate these, but they are not easy to grep...