Skip to content

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 requested a review from Carreau November 21, 2024 12:21
@trallard trallard changed the title 🔧 Update pre-commit config LINT - Run updated pre-commit hooks Nov 21, 2024
@Carreau Carreau force-pushed the run-precommit-hooks branch 2 times, most recently from fff8651 to e24e6d5 Compare November 21, 2024 14:13
@Carreau Carreau force-pushed the run-precommit-hooks branch from e24e6d5 to 02acb86 Compare November 21, 2024 14:18
@github-actions
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
Member

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
Member

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
Member

AlenkaF added a commit to apache/arrow that referenced this pull request Jul 2, 2025
…tion (#46883) ### Rationale for this change Stable and dev docs are not correctly updated and need manual intervention due to the [upstream change](pydata/pydata-sphinx-theme#2049) in version warning banner. ### What changes are included in this PR? Another sed substitution is added. ### Are these changes tested? I have tested locally with ``` > sed -i .bak -e '/^[[:space:]]\{8\}DOCUMENTATION_OPTIONS\.show_version_warning_banner =[[:space:]]*$/{ N s/^\([[:space:]]\{8\}DOCUMENTATION_OPTIONS\.show_version_warning_banner =[[:space:]]*\)\n[[:space:]]\{12\}false;/\1\n true;/g }' index.html ``` ### Are there any user-facing changes? No. * GitHub Issue: #45290 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
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

3 participants