-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Fix # optional tags #14184
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:7
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.
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. |
comment:8
Replying to @kcrisman:
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
OK, I'll have a look.
Doesn't matter.
OK, so that's a typo.
I have no idea.
I have no idea what these packages are.
OK, changing to all lowercase.
The doctester looks for "random", so it will be recognized. I think it's fine.
This can be seen as "just a comment" without further meaning.
I have no idea.
True, it's odd. I think it means that you need to run
At least with the new doctester (which is what we're aiming for),
That is also most certainly not the point of this ticket. |
This comment has been minimized.
This comment has been minimized.
Reviewer: Karl-Dieter Crisman |
comment:11
I figured :)
I was right - we did change it to the program needed.
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
It's ok, I think this is okay after looking at the various package lists.
Ok
Just wanted to make sure that additional comments were okay in doctest lines.
Got it - see the line above them (which your script didn't catch)?
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.
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)
So I believe that both of them should probably be made
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? |
This comment has been minimized.
This comment has been minimized.
comment:13
Replying to @kcrisman:
Done, changed all to
No, I used a double dash for consistency here.
Fixed.
Done.
Technically, it's worth a ticket but I have no plans to work on it. |
comment:14
Attachment: 14184_optional.patch.gz |
comment:15
Note that technically
Nor do I. |
comment:16
Judging from sage-devel, it seems that |
comment:17
Replying to @jdemeyer:
See this comment on Trac 9164. What do you think we should do? I'm almost wondering whether it's worth keeping your current patch and then opening a ticket acknowledging that this doesn't work on Cygwin... |
comment:18
But otherwise positive review. |
comment:19
I'd say leave this ticket as is and modify the code to try using time on Cygwin and suggesting installing the time package if it fails (or not using time=True) in another ticket. |
comment:20
Replying to @jpflori:
So in other words, not re-enabling optional, with the realization that we'll have a purposely failing doctest on Cygwin. I don't really have a problem with this. |
comment:21
Exactly, but opening a follow up ticket. |
comment:22
Since the "time" problem has nothing to do with Cygwin, I will keep the
tags. |
comment:24
Attachment: 14184_manual.patch.gz New version of attachment: 14184_manual.patch needs review. |
comment:25
Patchbot is confused... apply 14184_optional.patch and 14184_manual.patch |
This comment has been minimized.
This comment has been minimized.
comment:27
I found some more which I missed initially. |
Attachment: 14184_manual2.patch.gz |
comment:28
I'm sure there will always be more... |
Merged: sage-5.8.beta3 |
Execute
Apply attachment: 14184_optional.patch and attachment: 14184_manual.patch and attachment: 14184_manual2.patch
CC: @jpflori @dimpase
Component: doctest coverage
Author: Jeroen Demeyer
Reviewer: Karl-Dieter Crisman
Merged: sage-5.8.beta3
Issue created by migration from https://trac.sagemath.org/ticket/14184
The text was updated successfully, but these errors were encountered: