Skip to content

Fully-automatic clang-format with include reordering - #3713

Merged
rwgk merged 3 commits into
pybind:masterfrom
rwgk:clang-format_with_include_reordering
Feb 10, 2022
Merged

Fully-automatic clang-format with include reordering#3713
rwgk merged 3 commits into
pybind:masterfrom
rwgk:clang-format_with_include_reordering

Conversation

@rwgk

@rwgkrwgk commented Feb 10, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

To be merged after PR #3712.

Suggested changelog entry:

clang-format was added to the pre-commit actions, and the entire code base automatically reformatted (after several iterations preparing for this leap).

@rwgkrwgk mentioned this pull request Feb 10, 2022
Merged
@rwgkrwgk changed the title clang-format with include reordering (on top of #3708)Fully-automatic clang-format with include reorderingFeb 10, 2022
@rwgk
rwgk marked this pull request as ready for review February 10, 2022 01:36
@rwgk
rwgk requested a review from henryiii as a code ownerFebruary 10, 2022 01:36
@rwgk
rwgk requested a review from Skylion007February 10, 2022 01:36
henryiii
henryiii reviewed Feb 10, 2022
View reviewed changes
Comment thread.clang-format
Standard: Cpp11
StatementMacros: ['PyObject_HEAD']
TabWidth: 4
IncludeCategories:

@henryiiihenryiiiFeb 10, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This could go in before / be rebased in, doesn't need to be in the automatic commit, since we aren't running clang-format till this point.

@rwgkrwgkFeb 10, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Doing this right now.

Skylion007
Skylion007 reviewed Feb 10, 2022
View reviewed changes
Comment thread.clang-format
@@ -9,11 +9,28 @@ BinPackParameters: false
BreakBeforeBinaryOperators: All
BreakConstructorInitializers: BeforeColon
ColumnLimit: 99

@Skylion007Skylion007Feb 10, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i would add some comment pragmas for clang-tidy nolints

@rwgkrwgkFeb 10, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Do you have a pointer handy to some time-tested config? Or a concrete suggestion? Happy to fold that in. (I just don't want to invent an incompatible wheel, not currently needing anything in particular.)

@henryiiihenryiiiFeb 10, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CommentPragmas is a regular expression, can you just match it with a nolint comment? That's quite nice, actually.

@rwgkrwgk mentioned this pull request Feb 10, 2022
Merged
@rwgk
rwgkforce-pushed the clang-format_with_include_reordering branch 2 times, most recently from 98313de to c0fea50CompareFebruary 10, 2022 18:33
@rwgk

rwgk commented Feb 10, 2022

Copy link
Copy Markdown
CollaboratorAuthor

0497fd5

Nice! If the CI is happy here I'll cherry-pick that into #3712 (because we have all other .clang-format changes there, as suggested by @henryiii).

@rwgk

rwgk commented Feb 10, 2022

Copy link
Copy Markdown
CollaboratorAuthor

(The 🐍 3.5 • MSVC 2017 • x64 failure is an infrastructure flake, connection timed out. Safe to ignore.)

@rwgk
rwgkforce-pushed the clang-format_with_include_reordering branch from 0497fd5 to 4b1c3faCompareFebruary 10, 2022 19:51
@rwgk

rwgk commented Feb 10, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Screen Shot 2022-02-10 at 12 16 42

@rwgk
rwgk merged commit ec24786 into pybind:masterFeb 10, 2022
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Feb 10, 2022
@rwgk
rwgk deleted the clang-format_with_include_reordering branch February 10, 2022 21:29
This was referenced Feb 14, 2022
Merged
Closed
Merged
@jwnimmer-trijwnimmer-tri mentioned this pull request Mar 7, 2022
Closed
@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
@rwgkrwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Skylion007Skylion007Skylion007 left review comments
@henryiiihenryiiihenryiii 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.

3 participants

@rwgk@Skylion007@henryiii