Skip to content

Conversation

@user202729
Copy link
Contributor

@user202729 user202729 commented Aug 3, 2025

As in the title. See the code for detail.

📝 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

@github-actions
Copy link

github-actions bot commented Aug 3, 2025

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

@orlitzky
Copy link
Contributor

orlitzky commented Aug 9, 2025

Can you mention the defaults if replacement and replacement_rst_doc are left unspecified (=None)?

The generated HTML looks a little bit weird because sphinx puts parentheses after the method name when you use :meth:foo. It might look better if you wrote (for example) :meth:foo with algorithm="pari" instead, but this is just an idea. If you are happy with it, whatever, it's temporary.

Otherwise LGTM

@user202729
Copy link
Contributor Author

how about :meth:`frobenius_polynomial(algorithm="pari") <frobenius_polynomial>`​ (which will render as frobenius_polynomial(algorithm="pari") in a consecutive code block, but clicks through to frobenius_polynomial)?

@orlitzky
Copy link
Contributor

how about :meth:`frobenius_polynomial(algorithm="pari") <frobenius_polynomial>`​ (which will render as frobenius_polynomial(algorithm="pari") in a consecutive code block, but clicks through to frobenius_polynomial)?

That sounds even better, I didn't know that was an option.

@orlitzky
Copy link
Contributor

The new docs look great. Please still mention the defaults when replacement and replacement_rst_doc are left unspecified, but you can positive-review it after that.

@user202729
Copy link
Contributor Author

I guess https://github.com/sagemath/sage/pull/39161/files#diff-7647f4a9c65536028428ee1620ece71255c8c4ddc78ecb3db5540de2bb89d047R159 has something similar. (or maybe faster.)

haven't checked carefully.

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 12, 2025
sagemathgh-40528: Minor refactor for hyperelliptic curve
    
As in the title. See the code for detail.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] 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.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40528
Reported by: user202729
Reviewer(s): Michael Orlitzky, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 13, 2025
sagemathgh-40528: Minor refactor for hyperelliptic curve
    
As in the title. See the code for detail.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] 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.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40528
Reported by: user202729
Reviewer(s): Michael Orlitzky, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 14, 2025
sagemathgh-40528: Minor refactor for hyperelliptic curve
    
As in the title. See the code for detail.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] 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.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40528
Reported by: user202729
Reviewer(s): Michael Orlitzky, user202729
@vbraun vbraun merged commit faee909 into sagemath:develop Aug 16, 2025
22 checks passed
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.

4 participants