Opened 9 years ago
Last modified 9 years ago
#14184 closed defect
Fix # optional tags — at Version 12
Reported by: | jdemeyer | Owned by: | mvngu |
---|---|---|---|
Priority: | major | Milestone: | sage-5.8 |
Component: | doctest coverage | Keywords: | |
Cc: | jpflori, dimpase | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Execute
while read file; do if [ -h $file ]; then continue fi sed -i -f /dev/stdin $file <<'EOF' /sage: .*#/ { s/\(#.*\) needs optional/\1 optional -/i; s/\(#.*\) requires optional/\1 optional -/i; s/\(#\) requires[-:, ]*\([A-Za-z0-9_]*\)[-:, ]*optional/\1 optional - \2/i; s/\(#.*\) and optional/\1; optional/i; s/\(#.*\) \(random\) optional/\1 \2; optional/i; s/\(#.*\) \(long time\) optional/\1 \2; optional/i; s/\(#.*[^; ]\)[; ] *optional/\1; optional/i; s/\([#;,]\) *optional and /\1 optional; /i; s/\([#;,]\) *optional[-:, ]* needs /\1 optional - /i; s/\([#;,]\) *optional[-:, ]* requires /\1 optional - /i; s/\([#;,]\) *optional[-: ]* the /\1 optional - /i; s/\([#;,]\) *optional[-: ]* GAP packages/\1 optional - gap_packages/i; s/\([#;,]\) *optional[-: ]* GAP package \([A-Za-z0-9']*\)/\1 optional - gap_packages (\2 package)/i; s/\([#;,]\) *optional[-: ]* GAP \(optional \)*database[s]*/\1 optional - database_gap/i; s/\([#;,]\) *optional[-: ]* package/\1 optional -/i; s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) command/\1 optional - \2/i; s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) package/\1 optional - \2/i; s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) installed/\1 optional - \2/i; s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) and /\1 optional - \2 /i; s/\([#;,]\) *optional[-: ]* \([A-Za-z0-9_]*\) \(not tested\)/\1 optional - \2; \3/i; s/\([#;,]\) *optional[-: ]* internet connection/\1 optional - internet/i; s/\([#;,]\) *optional[-: ]* database_hohel/\1 optional - database_kohel/i; s/\([#;,]\) *optional[-: ]* Octave/\1 optional - octave/; s/\([#;,]\) *optional[-: ]* Axiom/\1 optional - axiom/; s/\([#;,]\) *optional[-: ]* \(.*\)'\(.*\)'/\1 optional - \2\3/i; T; s/\([#;,] *optional - .*[^# ]\)[# ] *$/\1/i; s/\([#;,] *optional\)[-: ][-: ]*$/\1/i; } EOF done < <(find devel/sage/sage devel/sage/doc/[a-z][a-z] -name '*.py*' -o -name '*.rst') # Fix specific files sed -i 's/# optional/# optional - gap_packages (Guava package)/' devel/sage/sage/coding/guava.py
Apply 14184_optional.patch and 14184_manual.patch
Change History (12)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
- Description modified (diff)
comment:5 Changed 9 years ago by
- Description modified (diff)
comment:6 Changed 9 years ago by
- Description modified (diff)
comment:7 follow-up: ↓ 8 Changed 9 years ago by
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 11 Changed 9 years ago by
Replying to kcrisman:
Isn't this exactly the kind of huge intrusive patch we've been trying to avoid? ;-)
Not really. Huge intrusive patches are okay as long as they serve a purpose. Removing whitespace all over the place doesn't serve a purpose, this patch certainly does.
This patch is mainly motivated by #12415, which has different (more strict) parsing for # optional
, leading to less false positives (but also less true positives, hence this patch).
- The sage/coding/guava.py should have the same message as the ones in sage/coding/code_bounds.py and sage/coding/linear_code.py
OK, I'll have a look.
# long time; optional - internet
and in another place, reversed - I assume that won't matter, but just asking in case
Doesn't matter.
# optional - database_hohel
- I don't know of a Hohel in Sage development, but I bet the database_kohel spkg is what is meant :-)
OK, so that's a typo.
- Did we change the
convert
ones toImageMagick
, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)
I have no idea.
- I'm not sure why something can depend on dot2tex but not graphviz, and even more weirdly, graphviz is experimental, but not dot2tex ?!?!? Though I guess it is okay...
I have no idea what these packages are.
- sometimes
octave
, sometimesOctave
- does capitalization matter? Same withAxiom
.
OK, changing to all lowercase.
somewhat random
- is that an official designation?
The doctester looks for "random", so it will be recognized. I think it's fine.
< 1/100th of a second
- well, I guess it might be! I guess we can leave that.
This can be seen as "just a comment" without further meaning.
- What is the
time
command? The one that we always have? I'm confused here.
I have no idea.
- The
sage/lfunctions/sympow.py
test seems ... odd. Should it instead be long test? Note that the previous command just says optional, with no indication of why.
True, it's odd. I think it means that you need to run sympow
first to create some tables, but I don't know more.
# optional - FES, random
- does it matter if random is before or after? I would think this is going to look for therandom
spkg. Same with# optional; randomly growing.
At least with the new doctester (which is what we're aiming for), optional - FES, random
will only require FES
.
Though obviously I am not going to try to test all of these optional tests by hand.
That is also most certainly not the point of this ticket.
comment:9 Changed 9 years ago by
- Description modified (diff)
comment:10 Changed 9 years ago by
- Status changed from new to needs_review
comment:11 in reply to: ↑ 8 Changed 9 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
This patch is mainly motivated by #12415, which has different (more strict) parsing for
# optional
, leading to less false positives (but also less true positives, hence this patch).
I figured :)
- Did we change the
convert
ones toImageMagick
, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)I have no idea.
I was right - we did change it to the program needed. search_src('ImageMagick')
gets me a lot of hits, particularly in plot/animate.py
plot/animate.py:430: sage: a.show() # optional -- ImageMagick plot/animate.py:435: sage: a.show(iterations=3) # optional -- ImageMagick plot/animate.py:439: sage: a.show(delay=50) # optional -- ImageMagick
I think this needs to be uniformized. See #11170, where this was done, except apparently to the guys here (see also #10655 and #7981 for background). By the way, is the --
versus -
important?
- I'm not sure why something can depend on dot2tex but not graphviz, and even more weirdly, graphviz is experimental, but not dot2tex ?!?!? Though I guess it is okay...
I have no idea what these packages are.
It's ok, I think this is okay after looking at the various package lists.
somewhat random
- is that an official designation?The doctester looks for "random", so it will be recognized. I think it's fine.
Ok
< 1/100th of a second
- well, I guess it might be! I guess we can leave that.This can be seen as "just a comment" without further meaning.
Just wanted to make sure that additional comments were okay in doctest lines.
- What is the
time
command? The one that we always have? I'm confused here.I have no idea.
Got it - see the line above them (which your script didn't catch)?
sage: v, t = qsieve(n, time=True) # uses the sieve (optional: time doesn't work on cygwin)
This line should be changed to "look nice", and now we know why this was marked optional. It hardly seems fair to make something optional that only doesn't work on a not-yet-supported platform... maybe we should remove these as optional? I'll leave that to you.
- The
sage/lfunctions/sympow.py
test seems ... odd. Should it instead be long test? Note that the previous command just says optional, with no indication of why.True, it's odd. I think it means that you need to run
sympow
first to create some tables, but I don't know more.
I believe you are right. But the doctest makes no sense as an automatic doctest in that case. See the relevant note. In fact, I get the bizarre (newlines not rendered)
sage: a = sympow.L(EllipticCurve('11a'), 2, 16); a "Inertia Group is C1 MULTIPLICATIVE REDUCTION\nConductor is 11\n**ERROR** P02L not found in param_data file\nIt can be added with './sympow -new_data 2'"
So I believe that both of them should probably be made optional - precomputations
, or perhaps even better to add
sage: sympow('-new_data 2') # not tested
to the doctest and make them all not tested, since by definition someone would have to do something "by hand" to make them testable. Or do you have another creative solution? I can think of a couple, but they are annoying and probably pointless.
There should also be better error catching there, but I figure anyone using sympow knows what they are doing enough to read the documentation - or is that worth a ticket as well?
comment:12 Changed 9 years ago by
- Description modified (diff)
Isn't this exactly the kind of huge intrusive patch we've been trying to avoid? ;-) That said, it would be really nice to have standardization for people to look at, so I'm not opposed to this.
Stuff to fix and/or think about doing as long as this ticket is opened.
# long time; optional - internet
and in another place, reversed - I assume that won't matter, but just asking in case# optional - database_hohel
- I don't know of a Hohel in Sage development, but I bet the database_kohel spkg is what is meant :-)convert
ones toImageMagick
, or vice versa? Anyway, that should be standardized if it wasn't already elsewhere (this feels familiar)octave
, sometimesOctave
- does capitalization matter? Same withAxiom
.somewhat random
- is that an official designation?< 1/100th of a second
- well, I guess it might be! I guess we can leave that.time
command? The one that we always have? I'm confused here.sage/lfunctions/sympow.py
test seems ... odd. Should it instead be long test? Note that the previous command just says optional, with no indication of why.# optional - FES, random
- does it matter if random is before or after? I would think this is going to look for therandom
spkg. Same with# optional; randomly growing.
Otherwise looks good - I did look at all of them by hand, not the script. Though obviously I am not going to try to test all of these optional tests by hand.