Document, lint, and fix placement of magic comments in multiline doctests #35181

mkoeppe · 2023-02-23T17:51:59Z

📚 Description

Fixes #33091

tldr:

# optional tags go on the sage: lines.
they don't go on the ....: lines.

📝 Checklist

I have made sure that the title is self-explanatory and the description concisely explains the PR.
I have linked an issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

codecov-commenter · 2023-02-24T06:20:58Z

Codecov Report

Base: 88.60% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (af479f2) compared to base (8f5bbd2).
Patch has no changes to coverable lines.

❗ Current head af479f2 differs from pull request most recent head 1308665. Consider uploading reports for the commit 1308665 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35181      +/-   ##
===========================================
- Coverage    88.60%   88.58%   -0.02%     
===========================================
  Files         2140     2140              
  Lines       397273   397272       -1     
===========================================
- Hits        352004   351937      -67     
- Misses       45269    45335      +66

Impacted Files	Coverage Δ
...omplex_reflection_or_generalized_coxeter_groups.py	`95.04% <ø> (ø)`
...age/categories/finite_complex_reflection_groups.py	`79.12% <ø> (ø)`
src/sage/combinat/abstract_tree.py	`96.04% <ø> (ø)`
src/sage/combinat/crystals/alcove_path.py	`93.72% <ø> (ø)`
src/sage/combinat/designs/bibd.py	`87.83% <ø> (ø)`
src/sage/combinat/designs/incidence_structures.py	`87.98% <ø> (ø)`
src/sage/combinat/designs/orthogonal_arrays.py	`90.42% <ø> (ø)`
...binat/designs/orthogonal_arrays_build_recursive.py	`85.32% <ø> (ø)`
src/sage/combinat/designs/resolvable_bibd.py	`97.16% <ø> (ø)`
...sage/combinat/designs/steiner_quadruple_systems.py	`95.68% <ø> (ø)`
... and 85 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dimpase · 2023-02-24T11:44:17Z

do you have 3a1a03a in the branch? Cause I don't get why you get a test failure. Do you misdetect GAP3 as present?

dimpase · 2023-02-24T11:54:54Z

err, still not enough # optional - gap3 tags.

3a1a03ae2cc (Travis Scrimshaw  2023-01-25 10:43:07 +0900  199)             sage: W = ReflectionGroup((3,1,2))            # optional - gap3
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  200)             sage: data = {w: [w.to_matrix(), w.to_matrix(on_space="dual")] for w in W}  # optional - gap3
3a1a03ae2cc (Travis Scrimshaw  2023-01-25 10:43:07 +0900  201)             sage: for w in W.iteration_tracking_words():  # optional - gap3
3a1a03ae2cc (Travis Scrimshaw  2023-01-25 10:43:07 +0900  202)             ....:     w.reduced_word()                    # optional - gap3
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  203)             ....:     mats = [w.to_matrix(), w.to_matrix(on_space="dual")]  # optional - gap3
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  204)             ....:     mats
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  205)             ....:     assert data[w] == mats

tobiasdiez · 2023-02-24T14:36:44Z

To reduce these missing optional directives, maybe block level directives would be a handy solution? https://xdoctest.readthedocs.io/en/latest/xdoctest.directive.html

dimpase · 2023-02-24T15:38:55Z

To reduce these missing optional directives, maybe block level directives would be a handy solution? https://xdoctest.readthedocs.io/en/latest/xdoctest.directive.html

looks interesting

dimpase · 2023-02-24T16:57:15Z

I don't know what's up with relint - has anyone tried running what it suggests?

mkoeppe · 2023-02-24T17:53:08Z

I don't know what's up with relint - has anyone tried running what it suggests?

If you're referring to the messages about namespace packages, this is done in #35105, #35106, #35107, which need review

mkoeppe · 2023-02-24T17:55:15Z

still not enough # optional - gap3 tags.

The new linting rule complains because there are too many of these tags, not too few!

mkoeppe · 2023-02-24T18:03:38Z

[...] maybe block level directives would be a handy solution? https://xdoctest.readthedocs.io/en/latest/xdoctest.directive.html

Thanks for the pointer. More expressive/powerful doctest annotations would of course be helpful. Not really for this ticket, which is about getting rid of too many (and badly placed) directives on multi-line doctests. But definitely for the work in #34998 for the modularization effort.

However, block- and function-scope # optional annotations have been discussed many times over the years, and were rejected / discussed to death each time. At least, we were able to get in file-level annotations (top of file # sage.doctest - optional ...).

dimpase · 2023-02-24T18:15:12Z

more tags were needed due to them missing in new develop branch - fixed that

…

On Fri, 24 Feb 2023, 17:55 Matthias Köppe, ***@***.***> wrote: still not enough # optional - gap3 tags. The new linting rule complains because there are *too many* of these tags, not too few! — Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHDWUU5PWG7GV6IP6XTWZDYY5ANCNFSM6AAAAAAVF6EJAY> . You are receiving this because you commented.Message ID: ***@***.***>

mkoeppe · 2023-02-24T18:17:10Z

Fixed what where?

dimpase · 2023-02-24T18:18:35Z

pushed straight to develop
added 2 # optional: gap3 tags

mkoeppe · 2023-02-24T18:22:32Z

These tags that you added in c017a6a are wrong.

mkoeppe · 2023-02-24T18:23:05Z

And CI did not fail with 10.0.beta2 -- https://github.com/sagemath/sage/actions/runs/4258115923

dimpase · 2023-02-24T18:27:35Z

what do you mean? there were errors there

…

On Fri, 24 Feb 2023, 18:22 Matthias Köppe, ***@***.***> wrote: These tags that you added in c017a6a <c017a6a> are wrong. — Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHEX76OGOB7P5URLJ7DWZD37JANCNFSM6AAAAAAVF6EJAY> . You are receiving this because you commented.Message ID: ***@***.***>

dimpase · 2023-02-24T18:28:44Z

On Fri, 24 Feb 2023, 18:23 Matthias Köppe, ***@***.***> wrote: And CI did not fail with 10.0.beta2 -- https://github.com/sagemath/sage/actions/runs/4258115923

yes it did fail, you are probably looking at a different run

…

— Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHEWIKJEMEOG2ZRV3O3WZD4BJANCNFSM6AAAAAAVF6EJAY> . You are receiving this because you commented.Message ID: ***@***.***>

dimpase · 2023-02-24T19:16:31Z

On Fri, 24 Feb 2023, 18:23 Matthias Köppe, @.> wrote: And CI did not fail with 10.0.beta2 -- https://github.com/sagemath/sage/actions/runs/4258115923
yes it did fail, you are probably looking at a different run
…
— Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJXYHEWIKJEMEOG2ZRV3O3WZD4BJANCNFSM6AAAAAAVF6EJAY . You are receiving this because you commented.Message ID: @.>

Dude, of course the build was OK, it's build+test that failed.

mkoeppe · 2023-02-24T19:19:17Z

I've pushed the correct fix.

mkoeppe · 2023-02-24T19:20:42Z

Could you please undo the vandalism on the develop branch?

dimpase · 2023-02-24T19:28:56Z

no idea what you are talking about. tags were missing there

mkoeppe · 2023-02-24T19:30:50Z

Read the documentation added here on the branch: https://github.com/sagemath/sage/pull/35181/files#diff-9a1298cd02d22076148519df77cb7b7a99cce074fe4a07c3f4aade37e821705aR1225

dimpase · 2023-02-24T22:45:14Z

OK, sorry for confusion. I fixed the develop branch, as you might have noticed.

github-actions · 2023-02-24T23:18:09Z

Documentation preview for this PR is ready! 🎉
Built with commit: 1308665

mkoeppe · 2023-02-24T23:19:49Z

Thanks! I've merged it already.

tornaria

LGTM

mkoeppe · 2023-02-26T01:46:45Z

Thank you!

vbraun · 2023-03-23T20:10:03Z

Merge conflict, wait for the next beta

There are a few tests in the sage library that span multiple lines and that were intended to be marked as "long time", but were not due to an implementation detail. As it stands, it is the FIRST line of a doctest that needs to be marked "long time", and not the last. This commit fixes a few doctests whose last (but not first) lines were tagged, updating them instead to bear the "# long time" tag on each line. This is overkill, but will save us time in the event that the parser is ever changed.

… go in multiline tests

…t output

… 'physical line'

…ional tags

mkoeppe · 2023-03-27T18:33:09Z

Trivial rebase, back to positive

mkoeppe added c: documentation c: doctest framework labels Feb 23, 2023

mkoeppe requested a review from seblabbe February 23, 2023 17:51

mkoeppe self-assigned this Feb 23, 2023

mkoeppe mentioned this pull request Feb 23, 2023

labels in multiline doctests #33091

Closed

mkoeppe added the s: needs review label Feb 23, 2023

mkoeppe force-pushed the t/33091/ticket/33091 branch from dfabd29 to 5dc0fbf Compare February 24, 2023 00:33

mkoeppe changed the title ~~Document and fix placement of magic comments in multiline doctests~~ Document, lint, and fix placement of magic comments in multiline doctests Feb 24, 2023

mkoeppe force-pushed the t/33091/ticket/33091 branch from af479f2 to 1913358 Compare February 24, 2023 19:17

mkoeppe requested a review from tornaria February 25, 2023 08:47

tornaria approved these changes Feb 26, 2023

View reviewed changes

tornaria added s: positive review and removed s: needs review labels Feb 26, 2023

vbraun removed the s: positive review label Mar 23, 2023

orlitzky and others added 14 commits March 27, 2023 11:27

src/doc/en/developer/coding_basics.rst: Explain where doctest markers…

6ca1f95

… go in multiline tests

src/.relint.yml (multiline_doctest_comment): New

297cc2e

Restore lost # indirect doctest

32c910f

src/doc/pt/tutorial/interfaces.rst: Fix indentation in doctests

f660705

Restore lost # indirect doctest, # known bug

7dd8336

src/doc/en/developer/coding_basics.rst: Expand an example, add doctes…

78608c6

…t output

src/doc/en/developer/coding_basics.rst: Refer to python reference for…

9377f4c

… 'physical line'

src/doc/en/developer/coding_basics.rst: Fix RST markup

7d0b783

src/sage/interfaces/kenzo.py: Fix up doctest markup

32c9b63

Fix magic comments in multiline doctests

116774e

src/sage/geometry/polyhedron/backend_normaliz.py: Fix typo

884cece

src/sage/combinat/root_system/reflection_group_element.pyx: Fix # opt…

2861163

…ional tags

mkoeppe force-pushed the t/33091/ticket/33091 branch from 1308665 to 2861163 Compare March 27, 2023 18:28

mkoeppe added the s: positive review label Mar 27, 2023

vbraun merged commit 0ae9698 into sagemath:develop Apr 1, 2023

mkoeppe added this to the sage-10.0 milestone Apr 1, 2023

mkoeppe removed the s: positive review label Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document, lint, and fix placement of magic comments in multiline doctests #35181

Document, lint, and fix placement of magic comments in multiline doctests #35181

mkoeppe commented Feb 23, 2023 •

edited

codecov-commenter commented Feb 24, 2023 •

edited

dimpase commented Feb 24, 2023

dimpase commented Feb 24, 2023

tobiasdiez commented Feb 24, 2023

dimpase commented Feb 24, 2023

dimpase commented Feb 24, 2023 •

edited

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023 via email

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023 via email

dimpase commented Feb 24, 2023 via email

dimpase commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023

github-actions bot commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

tornaria left a comment

mkoeppe commented Feb 26, 2023

vbraun commented Mar 23, 2023

mkoeppe commented Mar 27, 2023

Document, lint, and fix placement of magic comments in multiline doctests #35181

Document, lint, and fix placement of magic comments in multiline doctests #35181

Conversation

mkoeppe commented Feb 23, 2023 • edited

📚 Description

📝 Checklist

⌛ Dependencies

codecov-commenter commented Feb 24, 2023 • edited

Codecov Report

dimpase commented Feb 24, 2023

dimpase commented Feb 24, 2023

tobiasdiez commented Feb 24, 2023

dimpase commented Feb 24, 2023

dimpase commented Feb 24, 2023 • edited

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023 via email

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023 via email

dimpase commented Feb 24, 2023 via email

dimpase commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

dimpase commented Feb 24, 2023

github-actions bot commented Feb 24, 2023

mkoeppe commented Feb 24, 2023

tornaria left a comment

Choose a reason for hiding this comment

mkoeppe commented Feb 26, 2023

vbraun commented Mar 23, 2023

mkoeppe commented Mar 27, 2023

mkoeppe commented Feb 23, 2023 •

edited

codecov-commenter commented Feb 24, 2023 •

edited

dimpase commented Feb 24, 2023 •

edited