Skip to content

chore: apply CI for clang-format only for dash files #5949

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
Merged
PastaPastaPasta merged 6 commits into from
Apr 2, 2024
Merged

chore: apply CI for clang-format only for dash files #5949

PastaPastaPasta merged 6 commits into from
Apr 2, 2024

Conversation

@knst
Copy link
Collaborator

@knst knst commented Mar 21, 2024 •
edited
Loading

Issue being fixed or feature implemented

Note: should be this PR either #5942 be merged, not both

CI clang-format triggers to non-dash files + clang format is differ from out current formatting.

What was done?

See each commits

How Has This Been Tested?

See CI result
To test locally how new style will look, just run this command:

diff -u <(cat {coinjoin,governance,llmq,evo,masternode}/*.{h,cpp}) <(clang-format-16 {coinjoin,governance,llmq,evo,masternode}/*.{h,cpp} )

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst added this to the 21 milestone Mar 21, 2024
@knst knst force-pushed the fix-clang-format branch 2 times, most recently from 40e7663 to b42c2ff Compare March 21, 2024 21:25
@knst knst requested review from PastaPastaPasta and UdjinM6 March 21, 2024 21:26
@knst knst mentioned this pull request Mar 21, 2024
Closed
5 tasks
UdjinM6
UdjinM6 reviewed Mar 22, 2024
View reviewed changes
@knst
Copy link
Collaborator Author

knst commented Mar 22, 2024

without this one, new clang config is trying to contact lines of enums like that:

 // FOR SEEN MAP ARRAYS - GOVERNANCE OBJECTS AND VOTES
-enum class SeenObjectStatus {
-    Valid = 0,
-    ErrorInvalid,
-    Executed,
-    Unknown
-};
+enum class SeenObjectStatus { Valid = 0, ErrorInvalid, Executed, Unknown };

and

-enum class GovernanceObject : int {
-    UNKNOWN = 0,
-    PROPOSAL,
-    TRIGGER
+enum class GovernanceObject : int { UNKNOWN = 0, PROPOSAL, TRIGGER };

that's not a single example.

But I am open for proposes how to avoid this suggestion by the better way.
+enum class GovernanceObject : int { UNKNOWN = 0, PROPOSAL, TRIGGER }; is too bad IMO

@knst knst requested a review from UdjinM6 March 22, 2024 20:21
@UdjinM6
Copy link

UdjinM6 commented Mar 23, 2024

pls check d1a63111a3

@knst knst force-pushed the fix-clang-format branch from b42c2ff to 753af70 Compare March 23, 2024 11:56
UdjinM6
UdjinM6 previously approved these changes Mar 23, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@knst
Copy link
Collaborator Author

knst commented Mar 23, 2024

as pasta noticed:

     CPendingDsaRequest(CService addr_, CCoinJoinAccept dsa_) :
-        addr(std::move(addr_)),
-        dsa(std::move(dsa_)),
-        nTimeCreated(GetTime())
+        addr(std::move(addr_)), dsa(std::move(dsa_)), nTimeCreated(GetTime())
     {
     }

that's not expecting changes

@knst knst marked this pull request as draft March 23, 2024 17:12
@knst knst force-pushed the fix-clang-format branch from 753af70 to 7ef5249 Compare March 23, 2024 17:41
@knst knst marked this pull request as ready for review March 23, 2024 17:43
@UdjinM6
Copy link

UdjinM6 commented Mar 23, 2024

a couple more suggestions 8b3b800

@knst knst force-pushed the fix-clang-format branch from 7ef5249 to adc0e4b Compare March 24, 2024 19:53
@knst knst requested a review from UdjinM6 March 24, 2024 19:54
UdjinM6
UdjinM6 approved these changes Mar 25, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

light ACK, not 100% accurate but much better than before imo

@PastaPastaPasta PastaPastaPasta merged commit 85caf8a into dashpay:develop Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

21

Development

Successfully merging this pull request may close these issues.

3 participants

@knst @UdjinM6 @PastaPastaPasta