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

Reformat #1218

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Reformat #1218

merged 3 commits into from
Jul 11, 2024

Conversation

trws
Copy link
Member

@trws trws commented Jun 6, 2024

Two small tweaks to add this to the format script and remove the special-casing for pack/unpack, then apply formatting. Once we're happy with this, I intend to add a .git-ignore-revs or similar file we can use to ignore the global reformat commit in blame, but it has to have the commit sha in full in the file.

@trws
Copy link
Member Author

trws commented Jun 7, 2024

Now that the release is together, I rebased the reformat here. Short of adding CI to verify it, which might accidentally work because it's in the format script now that I think about it, this should be good to go.

@trws trws force-pushed the reformat branch 2 times, most recently from 78b80b1 to a24d061 Compare July 8, 2024 19:02
@trws
Copy link
Member Author

trws commented Jul 8, 2024

@milroy, what do you think of just getting this in? I've rebased and reapplied the formatting. I've also created a pull request to your rank-based-partial-release branch with a rebase of your branch on top of this with the reformatting applied to every commit to preserve all your changes. If you have new changes that aren't in there, or just want to do it yourself, this script completely automates it assuming you have fetched my reformat branch and are currently on your "partial-release" branch, or any other branch for that matter:

export BEFORE_MASS_FORMAT=af44d3b53457240d511bd2a32a3485fbdb91246d
export MASS_FORMAT=78b80b195bc36186abb438f499adbee5bef4a025
git rebase \
  --strategy-option theirs \
  --empty=drop \
  --exec "$(echo './scripts/format' \
      '|| echo prettier failed its ok; ' \
      'git commit -a --amend --no-verify --no-edit' \
    )" \
  --onto $MASS_FORMAT $BEFORE_MASS_FORMAT;

This is also now enforced in CI in this PR.

@trws trws force-pushed the reformat branch 2 times, most recently from 8b8e34e to ffb487f Compare July 8, 2024 19:44
@trws
Copy link
Member Author

trws commented Jul 8, 2024

Apologies if anyone looked at this during the churn after the last comment. Had to switch our format infrastructure to use pre-commit like core does to ensure we get consistent versions and avoid formatting differences from that. Now all straightened up, and using clang-format 18.1.6.

@trws
Copy link
Member Author

trws commented Jul 8, 2024

Reminder to self: When we're ready to pull the trigger here, I need to change the name of the action in the branch protection rules so this and other PRs with the new name can pass.

@milroy
Copy link
Member

milroy commented Jul 9, 2024

Sorry for the delay. I'll review this PR tonight.

Copy link
Member

@milroy milroy left a comment

Choose a reason for hiding this comment

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

This will be very helpful for the long-term maintenance of Fluxion.

Let's get it merged.

@milroy
Copy link
Member

milroy commented Jul 9, 2024

I've also created a pull request to your rank-based-partial-release branch with a rebase of your branch on top of this with the reformatting applied to every commit to preserve all your changes. If you have new changes that aren't in there, or just want to do it yourself, this script completely automates it assuming you have fetched my reformat branch and are currently on your "partial-release" branch, or any other branch for that matter

Thanks a lot for providing an automated way to update my PR. I've made a few changes to the branch so I I'll run the command myself.

@trws trws added merge-when-passing mergify.io - merge PR automatically once CI passes and removed merge-when-passing mergify.io - merge PR automatically once CI passes labels Jul 9, 2024
Copy link
Member

@jameshcorbett jameshcorbett left a comment

Choose a reason for hiding this comment

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

I noticed two things that could maybe be squashed out of existence. Otherwise looks great. I'm wondering whether it needs to be merged manually since it expects the 'python format' action which has been renamed? In which case I'm happy to do that

scripts/format Outdated Show resolved Hide resolved
scripts/check-format Outdated Show resolved Hide resolved
@trws trws force-pushed the reformat branch 2 times, most recently from 6e849da to 095dadd Compare July 11, 2024 16:09
problem: we were special-casing pack and unpack functions to prevent
them from being formatted

solution: stop special-casing them
@trws trws added the merge-when-passing mergify.io - merge PR automatically once CI passes label Jul 11, 2024
trws added 2 commits July 11, 2024 13:25
problem: we don't automatically check c/c++ formatting on push

solution: add clang-format to the check-format script

I also renamed "python formatting" to "code formatting" here so that
would stay accurate
problem: we spend a lot of time on code formatting, and have a somewhat
inconsistent style

solution: automate it with clang-format
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 79.58225% with 391 lines in your changes missing coverage. Please review.

Project coverage is 74.4%. Comparing base (fc4fcde) to head (806f8fc).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #1218    +/-   ##
=======================================
  Coverage    74.3%   74.4%            
=======================================
  Files         104     104            
  Lines       15184   14936   -248     
=======================================
- Hits        11295   11118   -177     
+ Misses       3889    3818    -71     
Files Coverage Δ
qmanager/modules/qmanager_opts.hpp 100.0% <ø> (ø)
...anager/policies/queue_policy_conservative_impl.hpp 76.0% <100.0%> (-1.0%) ⬇️
qmanager/policies/queue_policy_easy_impl.hpp 100.0% <ø> (ø)
qmanager/policies/queue_policy_factory_impl.hpp 84.2% <100.0%> (-0.8%) ⬇️
qmanager/policies/queue_policy_fcfs.hpp 100.0% <100.0%> (ø)
qmanager/policies/queue_policy_hybrid_impl.hpp 69.2% <100.0%> (-2.2%) ⬇️
resource/evaluators/expr_eval_target.hpp 100.0% <ø> (ø)
resource/evaluators/expr_eval_vtx_target.cpp 89.4% <100.0%> (-0.4%) ⬇️
resource/evaluators/expr_eval_vtx_target.hpp 100.0% <ø> (ø)
resource/evaluators/test/expr_eval_test01.cpp 100.0% <100.0%> (ø)
... and 91 more

@mergify mergify bot merged commit bc9e3e3 into flux-framework:master Jul 11, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing mergify.io - merge PR automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants