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

maint: Ignore formatting changes in history #3738

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

mathbunnyru
Copy link
Contributor

@mathbunnyru mathbunnyru commented Jan 12, 2025

No description provided.

.git-blame-ignore-revs Outdated Show resolved Hide resolved
.git-blame-ignore-revs Outdated Show resolved Hide resolved
@mathbunnyru
Copy link
Contributor Author

Not allowing to both change style and something meaningful in the same PR is definitely up to reviewers.

@jjerphan jjerphan added the release::ci_docs For PRs related to CI or documentation label Jan 13, 2025
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM modulo a title change to something like "maint: Ignore formatting changes in history" since this ignores more than those two files.

.git-blame-ignore-revs Outdated Show resolved Hide resolved
.git-blame-ignore-revs Outdated Show resolved Hide resolved
@mathbunnyru mathbunnyru changed the title Update .git-blame-ignore-revs for .pre-commit-config.yaml and .clang-… maint: Ignore formatting changes in history Jan 13, 2025
@mathbunnyru mathbunnyru force-pushed the update_git_blame_ignore_revs branch from 35911ca to f5a2529 Compare January 13, 2025 08:17
@mathbunnyru
Copy link
Contributor Author

@jjerphan I rebased the changes, and added prettier entry as well.

Copy link
Member

@Hind-M Hind-M left a comment

Choose a reason for hiding this comment

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

Thank you for handling this!

# 7d6856e9b56fbff35a6563889a737824054cf51f
# Add CondaURL (#2805)
# a527511c14ade1db03b3a88cf9c603eb35469663
# bump clang-format to 13.0.1 and make it work again
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# bump clang-format to 13.0.1 and make it work again
# bump clang-format to 13.0.1 and make it work again (PR number)

Can we add the PR number like in the others? (some numbers are missing here and there below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I don't want to manually add PR numbers to other places - I don't see a lot of sense, and it will be more difficult to update this file later (more manual changes - less clear it is).

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we shouldn't do this manually, but I'm wondering why it was added automatically in some and not others? Was it because they were all not PRs?

Copy link
Contributor Author

@mathbunnyru mathbunnyru Jan 13, 2025

Choose a reason for hiding this comment

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

Some of them were PRs.

Until 2022-10-26, some PRs were not squashed when merged to the main branch.
If you squash merge, GitHub automatically adds a PR number to the commit message.
If you don't, then you get the original commit and the original commit message (so there will be no PR number).

You can notice, that the lines without PR numbers are always at the bottom of the lists, so we won't have such a problem in the future (unless someone forgets to squash when merging the PR).


# Enforce space between definitions (#3049)
21675b6517ff5da3c4dc91b9083366136308db2d
# Attempt to fix CI (#2637)
50a627e002d8260fa2e116e5833190a135aff4e8
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be more than formatting, so should be commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is 99.9% formatting: 50a627e
Moreover, this commit has been ignored for several years:

50a627e002d8260fa2e116e5833190a135aff4e8

(Don't look at the wrong commit message in main, but exactly this commit hash is already ignored in main).

Copy link
Member

@Hind-M Hind-M left a comment

Choose a reason for hiding this comment

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

Thanks!

@mathbunnyru
Copy link
Contributor Author

mathbunnyru commented Jan 13, 2025

@Hind-M @JohanMabille @jjerphan I forgot to tell you — prettier has been merged to main today, and I highly recommend you merge main in all the current PRs touching at least one non-python or non-cpp file.
Otherwise, there is a chance these PRs have the wrong styling and you will have to fix main after merging these PRs.

And to make it even better, I would also recommend adding https://pre-commit.ci as an app:

  • it runs faster
  • applies autofixes in PRs (when tools have autofixes).
  • you can auto-update hooks, which is quite nice

We've been using it for years across several jupyter projects - it works really well.
I can start an issue for that, but I won't be able to add this tool myself - you need org-level permissions to do that.

Updated: an issue: #3741

@jjerphan jjerphan merged commit dead2e6 into mamba-org:main Jan 13, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::ci_docs For PRs related to CI or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants