sage.geometry: Update # needs, use block-scoped tags#36033
sage.geometry: Update # needs, use block-scoped tags#36033vbraun merged 9 commits intosagemath:developfrom
sage.geometry: Update # needs, use block-scoped tags#36033Conversation
…robe all src/sage/geometry
|
For instance, |
|
|
|
OK. There are a few warnings |
|
How did they escape from the scrutiny of |
|
These are false warnings from the doctester. I'll investigate what's going wrong there. |
|
Yes, it's a bug in the doctest parser that affects blocks with continuation lines: The block tag is forgotten immediately after the |
|
This is probably from the lines in src/sage/doctest/parsing.py:1092. I'll try to fix this tomorrow. |
|
OK. |
|
I investigated this issue. The "hack" implements the line continuation backslash. See #12415. So the "hack" removes the line continuation backslash so that the doctester accepts the doctest. In doing this, it inserts a newline character The added newline character effectively inserts a blank line between two doctest lines, so that the doctester cancels the persistent needs tag. That is why we get the warning message. Then what should be a cure? It seems that we have two choices. (1) (2) We "fix" the "hack" by --- a/src/sage/doctest/parsing.py
+++ b/src/sage/doctest/parsing.py
@@ -1087,13 +1087,8 @@ class SageDocTestParser(doctest.DocTestParser):
# doctest system.
m = backslash_replacer.search(string)
while m is not None:
- next_prompt = find_sage_prompt.search(string, m.end())
g = m.groups()
- if next_prompt:
- future = string[m.end():next_prompt.start()] + '\n' + string[next_prompt.start():]
- else:
- future = string[m.end():]
- string = string[:m.start()] + g[0] + "sage:" + g[1] + future
+ string = string[:m.start()] + g[0] + "sage:" + g[1] + string[m.end():]
m = backslash_replacer.search(string, m.start())
replace_ellipsis = not python_prompt.search(string)Thus we prevent spurious new lines from being added. I think we should choose one that has less side effects. |
|
By some experiments, it seems that (1) is not an option. Line continuation backslash doesn't work at all without the "hack"... |
|
(2) looks like a good solution to me |
|
I agree. There are just |
|
OK, found them and fixed them in #36034. |
| 1 | ||
| sage: Q.integral_points_count(explicit_enumeration_threshold=0) # optional - latte_int | ||
| sage: Q.integral_points_count(explicit_enumeration_threshold=0) | ||
| 1 |
There was a problem hiding this comment.
This is a removal of a tag. Is this correct? Was it a misplaced tag or something has changed meanwhile?
There was a problem hiding this comment.
This should not have been removed. I'll investigate.
There was a problem hiding this comment.
No, the change is actually correct. This example is handled by preprocessing and does not need latte_int.
|
I looked through the changes, which were made by, I guess, If |
I am creating these PRs by:
I'd say it's very reliable now -- within its specifications:
So eyeballing the changes is still necessary. I do this when I look for opportunities for using more block tags, but it's easy to miss a few bad changes. |
|
Thanks for the summary of how you prepared the PR. All reasonable.
It seems so to me too.
I see. I am expecting too much if I think that an automation could go without supervision. |
|
Thank you! |
|
Documentation preview for this PR (built with commit 897660f; changes) is ready! 🎉 |
sagemathgh-36033: `sage.geometry`: Update `# needs`, use block-scoped tags <!-- ^^^^^ 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? --> Cherry-picked from - sagemath#35095 Part of - sagemath#29705 <!-- 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. - [ ] 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 - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36033 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
sagemathgh-36034: Fix block-scoped doctest tags with `\` line continuations <!-- ^^^^^ 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? --> Fixes sagemath#36033 (comment) <!-- 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. - [ ] 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 - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> - Depends on sagemath#36025 (merged here) <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36034 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Matthias Köppe
Cherry-picked from
Part of
📝 Checklist
⌛ Dependencies