Skip to content

Removing AlignConsecutiveAssignments: true. #3067

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

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jun 30, 2021

Based on discussion in 2021-06-29 meeting between @EricCousineau-TRI, @henryiii, @rwgk, @Skylion007.

Proposed by rwgk, based on experience clang-formatting the smart_holder code:

  • Removal of AlignConsecutiveAssignments.

Motivation: Avoid git diff whitespace clutter. See example below.

This also brings us closer to the pure LLVM style.

While we're at it: also cleaning out a commented-out override. (If we really want it in the future it's easy enough to add back. In the meantime the config file is cleaner.)

Note: rwgk will reformat the smart_holder code after this PR is merged. Currently the master branch is not directly affected, although this PR is meant to be a preparation for globally clang-formatting all pybind11 code (after the next release).

See also: https://github.com/pybind/pybind11/wiki/Roadmap

$ git diff
diff --git a/dummy.cpp b/dummy.cpp
index fd7d1ad8..279eb5ea 100644
--- a/dummy.cpp
+++ b/dummy.cpp
@@ -1,5 +1,6 @@
-int value1 = 1;
-int value2 = 2;
-int value3 = 3;
-int value4 = 4;
-int value5 = 5;
+int value1        = 1;
+int value2        = 2;
+int value3        = 3;
+int value4        = 4;
+int value5        = 5;
+int value_special = 99;

Also removing a commented-out line.
@EricCousineau-TRI
Copy link
Collaborator

Just curious - what's the functional change here? Shouldn't this change involve formatting change as in #3065? (sorry for being slow!)

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 30, 2021

Just curious - what's the functional change here? Shouldn't this change involve formatting change as in #3065? (sorry for being slow!)

I want to be sure we agree on the format before making changes on the smart_holder branch, by way of having the agreed-on format on master first, even though it's currently not applied here.

What I had in mind for after we agree:

From then on all pybind11 code will be clang-format clean, enforced automatically.

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 30, 2021

Interesting CI flake: one of the two AppVeyor jobs stalled in the middle of running the Python unit tests and then timed out after one hour. I've not seen this before. In any case, it's certainly unrelated to the .clang-format change!

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 2, 2021

Hi @EricCousineau-TRI based on your comment under #3065 (#3065 (comment)) I'll go ahead merging this PR.

Next steps:

@rwgk rwgk merged commit 795e3c4 into pybind:master Jul 2, 2021
@rwgk rwgk deleted the dot_clang_format_tweak branch July 2, 2021 21:14
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 2, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jul 2, 2021
rwgk added a commit that referenced this pull request Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants