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

Correct developer guide's function template #38068

Merged
merged 5 commits into from
Jun 1, 2024

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented May 24, 2024

Add spaces in tuples.

P.S. Should the default values 1 and 2 have single or double backticks?

Copy link

github-actions bot commented May 24, 2024

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

@saraedum
Copy link
Member

saraedum commented May 24, 2024

The 1 and 2 should have no backticks at all. They could also have two but I don't think that LaTeX typeset makes sense here.

I think the 1 and 2 should have two backticks here. No backticks is also fine, but it's easier to teach people to use `` here.

@gmou3
Copy link
Contributor Author

gmou3 commented May 24, 2024

I simplified a bit the examples, and changed the note. Previously, the note was more like an algorithm block for which we have a good example.

@mkoeppe mkoeppe requested a review from tscrim May 26, 2024 22:24
Copy link
Member

@saraedum saraedum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood where this goes in the entire documentation. I think the original OUTPUT was actually fine.

Then again, we have many examples of OUTPUT: tuple in the code base. So it's a good template to follow that.

@gmou3
Copy link
Contributor Author

gmou3 commented May 27, 2024

Thank you.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 29, 2024
    
Add spaces in tuples.

P.S. Should the default values `1` and `2` have single or double
backticks?
    
URL: sagemath#38068
Reported by: gmou3
Reviewer(s): Julian Rüth
@gmou3
Copy link
Contributor Author

gmou3 commented May 31, 2024

@kwankyu I agree with these recommendations. It is clearer if one thinks of something more complicated as the default value, a field for example. We would like to show some nice math instead of some code I presume (the code would appear further up in the arguments).

I think I need to do a follow-up PR.

@kwankyu
Copy link
Collaborator

kwankyu commented May 31, 2024

If you agree, I may do it in #38117.

@saraedum
Copy link
Member

saraedum commented May 31, 2024

I'm confused. Should the defaults be typeset with single backticks and not double-backticks anymore?

I find 218 single backticks for defaults and 6567 double backticks for defaults in our codebase.

@gmou3
Copy link
Contributor Author

gmou3 commented May 31, 2024

Something from Python like None, or True, should have double backticks.
Something mathematical should have single backticks.

Let us know of any counter-argument.

@saraedum
Copy link
Member

saraedum commented May 31, 2024

Something from Python like None, or True, should have double backticks. Something mathematical should have single backticks.

Let us know of any counter-argument.

My main argument is that the code base uses more double-backticks than single-backticks. I believe that this document should not set new standards but report what the code base is already doing.

Also, 1 is the actual default value as a Python int, it's not really a "mathematical object" arguably. I believe that the defaults should report the actual default of a function, imho this can never be a mathematical object. If it's None and None means the ring of integers, then INPUT: should say so instead of claiming that ring (default: `\ZZ`) simply because `\ZZ` is not something you can type into SageMath. I have no strong opinion on that latter part though.

There's of course the caveat in my argument that 1 is the Python int and not the SageMath preparsed int but we tend to gloss over this oddity almost everywhere in the documentation (and our code tries to treat both of them equally.)

@gmou3
Copy link
Contributor Author

gmou3 commented May 31, 2024

I think the part after the "--" should be treated the same as the text body preceding the "INPUT". One can see how to type the default value from the function header.

I am unsure about what the rule is/was. The template had (and still has) single backticks however. We can determine here what is desirable based on some logic, and everything else can be adapted to comply.

@vbraun vbraun merged commit 2423da6 into sagemath:develop Jun 1, 2024
18 of 21 checks passed
@gmou3 gmou3 deleted the developer_guide_function_template branch June 1, 2024 22:52
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 1, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 4, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

which I think are already established.  But it is good to recommend
preferred styles to keep docstrings in consistent styles.

motivated by the comment:
sagemath#37821 (comment)

### 📝 Checklist

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

- [x] The title is concise and informative.
- [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.
- [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: ... -->

sagemath#38068 (merged here)
    
URL: sagemath#38117
Reported by: Kwankyu Lee
Reviewer(s): gmou3, Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 5, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

which I think are already established.  But it is good to recommend
preferred styles to keep docstrings in consistent styles.

motivated by the comment:
sagemath#37821 (comment)

### 📝 Checklist

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

- [x] The title is concise and informative.
- [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.
- [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: ... -->

sagemath#38068 (merged here)
    
URL: sagemath#38117
Reported by: Kwankyu Lee
Reviewer(s): gmou3, Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 8, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

which I think are already established.  But it is good to recommend
preferred styles to keep docstrings in consistent styles.

motivated by the comment:
sagemath#37821 (comment)

### 📝 Checklist

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

- [x] The title is concise and informative.
- [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.
- [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: ... -->

sagemath#38068 (merged here)
    
URL: sagemath#38117
Reported by: Kwankyu Lee
Reviewer(s): gmou3, Kwankyu Lee
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.

5 participants