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

LINT - Run updated pre-commit hooks #2049

Merged
merged 9 commits into from
Nov 21, 2024
Merged

Conversation

trallard
Copy link
Collaborator

@trallard trallard commented Nov 21, 2024

Follows-up #2044

@trallard trallard added the kind: maintenance Improving maintainability and reducing technical debt label Nov 21, 2024
@trallard trallard changed the title 🔧 Update pre-commit config LINT - Run updated pre-commit hooks Nov 21, 2024
tests/test_build.py Outdated Show resolved Hide resolved
@Carreau Carreau force-pushed the run-precommit-hooks branch 2 times, most recently from fff8651 to e24e6d5 Compare November 21, 2024 14:13
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/pydata_sphinx_theme
  short_link.py
  utils.py
Project Total  

This report was generated by python-coverage-comment-action

@Carreau Carreau merged commit f6102b4 into pydata:main Nov 21, 2024
25 checks passed
@trallard
Copy link
Collaborator Author

Thanks for your help @Carreau these are some of the most annoying PRs 💜 I am very grateful for you

@drammock
Copy link
Collaborator

This approach to git-blame-ignore-revs doesn't work. See:

https://github.com/pydata/pydata-sphinx-theme/blame/main/docs/conf.py#L197-L198

The problem is that the PR was squash-merged, so the referenced commit hashes don't exist on main. When you're contending with pre-commit CI, the way I've seen it handled is to:

  1. make a commit that only enables the new style rules and commit it with --no-verify
  2. run pre-commit locally, then stage and commit the auto-changes
  3. make a PR of those 2 commits, but then rebase-merge instead of squash-merge (needs to be temporarily enabled by repo admin in the settings)
  4. make a new PR that adds the commit hashes to git-blame-ignore-revs

In principle it should be possible to do it all as one PR (with 3 commits: enable-the-rule, apply-the-rule, and add-hash-to-ignore) and then rebase-merge that PR. But I've seen such attempts go sideways before --- maybe the commit hashes changed when rebased onto main? don't recall the details. The 2-PR approach is much less error-prone.

@Carreau
Copy link
Collaborator

Carreau commented Nov 22, 2024

My bad, I forgot that the default here is squash-merge – I guess that's one of the reason I dislike squash-merge.

And yes the commit hash do change when rebasing as the hash depends on all the parents.

@trallard trallard deleted the run-precommit-hooks branch November 25, 2024 12:38
@trallard
Copy link
Collaborator Author

Right, so would you suggest we revert this merge @drammock, or shall we do something else to fix this (I suppose we could add the squashed commit to the .git-blame-ignore-revs file, but that has the downside of ignoring all the changes, including updates to the pre-commit config file?

Also it might be worth adding #2049 (comment) to our dev docs

@drammock
Copy link
Collaborator

drammock commented Nov 26, 2024

I think what I would do is:

I can work on this now, and will try to preserve all the correct authorships.

drammock added a commit to drammock/pydata-sphinx-theme that referenced this pull request Nov 26, 2024
drammock added a commit to drammock/pydata-sphinx-theme that referenced this pull request Nov 26, 2024
drammock added a commit to drammock/pydata-sphinx-theme that referenced this pull request Nov 26, 2024
drammock added a commit to drammock/pydata-sphinx-theme that referenced this pull request Nov 26, 2024
drammock added a commit to drammock/pydata-sphinx-theme that referenced this pull request Nov 26, 2024
drammock added a commit that referenced this pull request Nov 26, 2024
This was referenced Nov 26, 2024
drammock added a commit that referenced this pull request Nov 26, 2024
@drammock
Copy link
Collaborator

All fixed! Now https://github.com/pydata/pydata-sphinx-theme/blame/main/docs/conf.py#L197-L198 points to #834 (2 years ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: maintenance Improving maintainability and reducing technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants