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

Normalize whitespace. #474

Merged
merged 4 commits into from
Apr 5, 2023
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jan 10, 2023

This PR re-enables pre-commit checks for normalizing whitespace (trimming trailing spaces) that I disabled in #471.

These pre-commit hooks existed prior to the GitHub Actions migration but they were not being enforced by CI style checks. I disabled them in #471 because enforcing them would have made the PR diff large.

The primary change is in .pre-commit-config.yaml, and everything else was automatically fixed by pre-commit.

# TODO: re-enable this after GitHub Actions migration, it currently makes a
# large number of changes that shouldn't go in the GitHub Actions PR.
#- repo: https://github.com/pre-commit/pre-commit-hooks
#  rev: v4.4.0
#  hooks:
#    - id: trailing-whitespace
#    - id: end-of-file-fixer
#    - id: debug-statements

@bdice bdice requested review from a team as code owners January 10, 2023 02:04
ajschmidt8
ajschmidt8 previously approved these changes Jan 10, 2023
@jakirkham jakirkham changed the base branch from branch-23.02 to branch-23.04 February 1, 2023 18:18
@jakirkham jakirkham dismissed ajschmidt8’s stale review February 1, 2023 18:18

The base branch was changed.

@jakirkham jakirkham added bug Something isn't working non-breaking Introduces a non-breaking change labels Feb 16, 2023
@jakirkham
Copy link
Member

Issue ( #484 ) has been addressed in branch-23.04. Have updated the PR branch here to merge in those changes

Also added labels to fix the label checker

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

The changes look good. Looks like the CI failure is probably an unrelated network issue (failed download). I restarted the failed jobs to see if it goes through on the second round.

@grlee77
Copy link
Contributor

grlee77 commented Feb 17, 2023

Thanks, @bdice

@jakirkham jakirkham closed this Apr 5, 2023
@jakirkham jakirkham reopened this Apr 5, 2023
@jakirkham
Copy link
Member

Toggling for CI

@jakirkham
Copy link
Member

@gpucibot merge

@rapids-bot
Copy link

rapids-bot bot commented Apr 5, 2023

@jakirkham, the @gpucibot merge command has been replaced with /merge.

Please re-comment this PR with /merge and use this new command in the future.

@jakirkham
Copy link
Member

/merge

@@ -14,4 +14,3 @@ index 7da31f7..5816de7 100644
+ //BOOST_ASSERT(0 == ((std::size_t)ret % ::boost::container::dtl::alignment_of<T>::value));
return static_cast<T*>(ret);
}

Copy link
Member

Choose a reason for hiding this comment

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

Think we want to keep this line. Otherwise the patch doesn't apply

Copy link
Member

Choose a reason for hiding this comment

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

Reverted in commit ( 43606cf )

@jakirkham
Copy link
Member

Looks like the style check doesn't like that change 😂

If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/cpp/cmake/deps/boost-header-only.patch b/cpp/cmake/deps/boost-header-only.patch
index e765790..96c0078 100644
--- a/cpp/cmake/deps/boost-header-only.patch
+++ b/cpp/cmake/deps/boost-header-only.patch
@@ -14,4 +14,3 @@ index 7da31f7..5816de7 100644
 +   //BOOST_ASSERT(0 == ((std::size_t)ret % ::boost::container::dtl::alignment_of<T>::value));
     return static_cast<T*>(ret);
  }
- 

Maybe we can skip this file during style checking?

.pre-commit-config.yaml Outdated Show resolved Hide resolved
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@rapids-bot rapids-bot bot merged commit 533fb2a into rapidsai:branch-23.04 Apr 5, 2023
@jakirkham
Copy link
Member

Thanks all! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants