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.

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

Make clang-format less strict #473

Closed
wants to merge 18 commits into from
Closed

Conversation

F1F7Y
Copy link
Member

@F1F7Y F1F7Y commented Jun 20, 2023

Docs: https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Makes clang-format less strict and also changes some things to make the code more readable ( this will very obviously require some parts to be changed manually ).

As I took my own spin on formatting and these changes are very individual based i expect this to take time, unlike other PRs.

@uniboi

This comment was marked as off-topic.

@F1F7Y F1F7Y marked this pull request as draft June 20, 2023 21:24
@F1F7Y F1F7Y changed the title Set clang-format ColumnLimit to 0 Make clang-format less strict + some changes Jun 21, 2023
@F1F7Y
Copy link
Member Author

F1F7Y commented Jun 21, 2023

also thinking about setting

PointerAlignment: Right
ReferenceAlignment: Right

@F1F7Y F1F7Y marked this pull request as ready for review June 21, 2023 18:01
.clang-format Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
@pg9182
Copy link
Member

pg9182 commented Jun 22, 2023

Can we limit this PR to relaxing rules rather than drastically changing formatting?

@F1F7Y F1F7Y changed the title Make clang-format less strict + some changes Make clang-format less strict Jun 23, 2023
Copy link
Member

@pg9182 pg9182 left a comment

Choose a reason for hiding this comment

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

Looks good in general.

NorthstarDLL/core/structs.h Outdated Show resolved Hide resolved
NorthstarDLL/util/version.cpp Show resolved Hide resolved
Copy link
Member

@pg9182 pg9182 left a comment

Choose a reason for hiding this comment

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

LGTM. There's some other clang-format settings we should change too, but this is good.

@F1F7Y F1F7Y added the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Jun 29, 2023
Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

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

Looks good but will probably cause conflicts in some PRs

@ASpoonPlaysGames
Copy link
Contributor

Before this gets merged someone should merge main into it, and then fix the formatting

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Sep 2, 2023
Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

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

re-reviewing since changes were made since my last review.

Still looks good, some additional format changes could be made but its best to split any more changes into new PRs

@GeckoEidechse
Copy link
Member

Quick skim and the changes are an improvement. That being said:

  1. I found out today that clang-format does tab indentation incorrectly, check my rant here Define indent size in editorconfig #550 (comment)
    (spaces superiority)
  2. When merging I will first merge only the config change and then the formatting changes in a separate commit so that I can then add that commit to the .git-blame-ignore-revs file so that git blame is still readable
  3. I'll probably merge the other PRs that are ready to merge first so that we don't cause a bunch of PRs that are ready to merge to merge conflict.

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Oct 17, 2023

For merging this we would first only merge the .clang-format related changes and then in a second commit, made by Northstar <[email protected]> we would apply the formatting changes so that we then can add the formatting commit to .git-blame-ignore-revs so that it doesn't show up in git blame ^^

So basically just revert the actual formatting changes (they are causing merge conflicts rn anyway) so that only the config changes are left and then we can merge this ^^

@GeckoEidechse GeckoEidechse added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Oct 17, 2023
@ASpoonPlaysGames ASpoonPlaysGames added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed READY TO MERGE This mergeable right now labels Oct 23, 2023
@F1F7Y
Copy link
Member Author

F1F7Y commented Aug 25, 2024

Superseded by #772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge merge conflicts Blocked by merge conflicts, waiting on the author to resolve
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants