-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
sage.sets
: Doctest cosmetics
#37713
sage.sets
: Doctest cosmetics
#37713
Conversation
I am against replacing short |
Documentation preview for this PR (built with commit 5ef67f5; changes) is ready! 🎉 |
Are you referring to copying from the source code or from the formatted HTML documentation? Where exactly does it hurt? |
Copying from source code. It does work, but it is a painful. For example, modifying does not work as well, and also emacs doesn't offer autocompletion for functions defined in this way. Also, I don't see a good reason for the replacement. What is the purpose? |
Coding style. In library code, such lambda assignments are flagged by pycodestyle, and they have been replaced already. Best to use a consistent style also in code examples. |
You know that you can copy the whole lines including prompts, yes? |
I would agree that in very many cases the assignment is unnecessary, and could simply be inlined. Otherwise, you could simply use the one-line form of
works just fine.
Is there an editor that would not let you copy full lines? |
We appear to be miscommunicating. What's the exact problem that you are facing when copying multi-line definitions? |
Codestyle linters also don't like that |
It is a bit hard to explain if you do not use emacs. I can go to the first line of the test in the documentation buffer, and press
If I do it this way (rather than copy-pasting), I at least get tab completion. However, if I now want to modify the definition slightly, it gets a bit cumbersome, at least for me. I also find that the increased verbosity is another drawback. I do see the point of using pycodestyle, but I would rather turn off that particular recommendation for doctests, or relax it to allow single line |
Is this |
Yes |
74aaf15
to
55a6b57
Compare
55a6b57
to
5ef67f5
Compare
I've reduced this PR to the subset of the changes that do not involve changing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Thanks. |
<!-- ^ 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". --> Cherry-picked from sagemath#35095. ### 📝 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. - [x] 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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37713 Reported by: Matthias Köppe Reviewer(s): Martin Rubey
<!-- ^ 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". --> Cherry-picked from sagemath#35095. ### 📝 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. - [x] 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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37713 Reported by: Matthias Köppe Reviewer(s): Martin Rubey
<!-- ^ 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". --> Cherry-picked from sagemath#35095. ### 📝 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. - [x] 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. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37713 Reported by: Matthias Köppe Reviewer(s): Martin Rubey
Cherry-picked from #35095.
📝 Checklist
⌛ Dependencies