Skip to content

Remove spurious diffs in doc previews #40595

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Aug 15, 2025

Again some spurious diffs crawled in. We remove them. For example,

https://doc-pr-40586--sagemath.netlify.app/changes

A sage doc needs to avoid release info in title. I suspect that release info in title is a source of spurious diffs. (I think I saw them in a doc preview changes somewhere, but I cannot find where now...) Anyway the release info is redundant.

Two unused conf.py are deleted.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Aug 15, 2025

Documentation preview for this PR (built with commit ad9c046; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu kwankyu marked this pull request as ready for review August 15, 2025 23:36
@user202729
Copy link
Contributor

does this fix #40530 (comment) also?

@user202729
Copy link
Contributor

I find filtering out all the 0x123456789ABCDEF possibly dangerous. It may be a better idea to use something like #38945 where the 0x123456789 does not show up in the documentation at all.

Also I don't understand what's the issue with release info, looking at some of the recent PR the "changes is ready!" button doesn't show any of these.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 16, 2025 via email

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 16, 2025

I find filtering out all the 0x123456789ABCDEF possibly dangerous. It may be a better idea to use something like #38945 where the 0x123456789 does not show up in the documentation at all.

The regex filters those only ending with ">".

Also I don't understand what's the issue with release info, looking at some of the recent PR the "changes is ready!" button doesn't show any of these.

Perhaps they don't cause diffs. But anyway they are redundant and look bad since the release info is just below the sage logo. They are duplicates. In pdf docs, correct release info appears anyway.

@user202729
Copy link
Contributor

user202729 commented Aug 16, 2025

(I think I saw them in a doc preview changes somewhere, but I cannot find where now...)

more likely possibility is that it's caused by #40379 instead (the documentation for pull request is based on e.g. 10.7.beta2, but the old doc that it's diffed against is based on 10.7.beta1). Either that or when the doc build for a certain pull request somehow finish before the doc build CI for the new release finish.

But if that information is redundant, is fine either way.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 18, 2025

(I think I saw them in a doc preview changes somewhere, but I cannot find where now...)

more likely possibility is that it's caused by #40379 instead (the documentation for pull request is based on e.g. 10.7.beta2, but the old doc that it's diffed against is based on 10.7.beta1). Either that or when the doc build for a certain pull request somehow finish before the doc build CI for the new release finish.

I see. Thanks.

But if that information is redundant, is fine either way.

For example, see https://doc-pr-40610--sagemath.netlify.app/html/hu/a_tour_of_sage/ (the left side).

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 21, 2025

needs more work?

I find filtering out all the 0x123456789ABCDEF possibly dangerous. It may be a better idea to use something like #38945 where the 0x123456789 does not show up in the documentation at all.

0x123456789 does not show up in the documentation. They are temporary changes to make genuine diffs.

Compare the doc preview of this PR

https://doc-pr-40595--sagemath.netlify.app/html/en/reference/algebras/sage/algebras/quatalg/quaternion_algebra#sage.algebras.quatalg.quaternion_algebra.normalize_basis_at_p

with that of #40644:

https://doc-pr-40644--sagemath.netlify.app/html/en/reference/algebras/sage/algebras/quatalg/quaternion_algebra#hunk1

@user202729
Copy link
Contributor

anyway, doesn't matter.

no, what I mean is we talked about the 0x123456789 showing up in the doc earlier #38945 , and you say that it's a better idea to just let the default argument be None then set it in the function instead.

image

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 21, 2025

anyway, doesn't matter.

no, what I mean is we talked about the 0x123456789 showing up in the doc earlier #38945 , and you say that it's a better idea to just let the default argument be None then set it in the function instead.

Ah, I see. Yes, it is better not to introduce 0x123456789 in the first place.

However, we (you and I) cannot prevent that some authors introduce them from time to time. Hence this measure.

I hope that the measure is more beneficial than dangerous.

Thanks for the review!

@user202729
Copy link
Contributor

However, we (you and I) cannot prevent that some authors introduce them from time to time. Hence this measure.

Hm… maybe a better measure is to make the CI fail loudly when such thing is introduced, rather than someone only notice it in the next release and have to "CI fix" it.

That can be left for later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants