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

Remove spurious diffs in doc build changes #36483

Merged
merged 26 commits into from
Oct 31, 2023

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Oct 19, 2023

Remove all spurious and chronic diffs in the changes of our documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. Scroll until vertical alignment breaks or search for some diff text.

In this regard, refrain from using a function as the default value of an argument in Python definitions. Use None instead and apply the default (the function) in the code part.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@kwankyu kwankyu changed the title remove spurious doc diffs Remove spurious diffs in doc build changes Oct 19, 2023
@kwankyu kwankyu marked this pull request as ready for review October 20, 2023 22:32
@kwankyu
Copy link
Collaborator Author

kwankyu commented Oct 20, 2023

Thanks. But I found a glitch meanwhile. Let me fix this first :-)

@kwankyu
Copy link
Collaborator Author

kwankyu commented Oct 21, 2023

Ready to go. Thanks!

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 21, 2023

needs rebase now

@kwankyu
Copy link
Collaborator Author

kwankyu commented Oct 21, 2023

Thanks. Merged with develop cleanly by git.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Oct 21, 2023

By the way, is there a reason that changes are only from html/en, that is, English doc, instead of html?

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 21, 2023

Only the English docs are uploaded to netlify, see the end of the "Copy docs" step.
I don't recall if this was discussed when the netlify deployment was developed.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Oct 21, 2023

Ah, right.

Then this is a bigger problem, to be discussed elsewhere. Thanks.

@github-actions
Copy link

Documentation preview for this PR (built with commit 67e7f95; changes) is ready! 🎉

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 21, 2023
    
Remove all spurious and chronic diffs in the changes of our
documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--
sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. Scroll until vertical
alignment breaks or search for some diff text.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36475

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36483
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 22, 2023

The improved CHANGES display is really nice.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Oct 22, 2023

Thanks. It would've been better if diffsite scrolls to (or highlight) diff parts. I found no way to do that automatically (perhaps need AI). But from my experience, the manual method in the description of this PR works well enough.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 24, 2023
    
Remove all spurious and chronic diffs in the changes of our
documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--
sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. *Scroll until vertical
alignment breaks or search for some diff text.*

In this regard, refrain from using a function as the default value of an
argument in Python definitions. Use `None` instead and apply the default
(the function) in the code part.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36475

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36483
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2023
    
Remove all spurious and chronic diffs in the changes of our
documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--
sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. *Scroll until vertical
alignment breaks or search for some diff text.*

In this regard, refrain from using a function as the default value of an
argument in Python definitions. Use `None` instead and apply the default
(the function) in the code part.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36475

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36483
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
    
Remove all spurious and chronic diffs in the changes of our
documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--
sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. *Scroll until vertical
alignment breaks or search for some diff text.*

In this regard, refrain from using a function as the default value of an
argument in Python definitions. Use `None` instead and apply the default
(the function) in the code part.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36475

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36483
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
    
Remove all spurious and chronic diffs in the changes of our
documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--
sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. *Scroll until vertical
alignment breaks or search for some diff text.*

In this regard, refrain from using a function as the default value of an
argument in Python definitions. Use `None` instead and apply the default
(the function) in the code part.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36475

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36483
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
    
Remove all spurious and chronic diffs in the changes of our
documentation preview.

Improve CHANGES.html using diffsite. See https://deploy-preview-36483--
sagemath-tobias.netlify.app/changes

diffsite allows to compare changes visually. *Scroll until vertical
alignment breaks or search for some diff text.*

In this regard, refrain from using a function as the default value of an
argument in Python definitions. Use `None` instead and apply the default
(the function) in the code part.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36475

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36483
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
@vbraun vbraun merged commit 83dcc17 into sagemath:develop Oct 31, 2023
12 of 20 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 31, 2023
@kwankyu kwankyu deleted the remove-spurious-doc-diffs branch December 13, 2023 11:48
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.

3 participants