-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
using argparse in sage-runtests script #32332
Comments
This comment has been minimized.
This comment has been minimized.
Branch: u/chapoton/32332 |
Commit: |
New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Reviewer: Travis Scrimshaw |
comment:4
LGTM as far as I can tell. A good ticket for the initial 9.5.beta0 release. |
comment:5
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:8
ok, should be better now. I fixed the behaviour of -p and --logfile. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:10
There are still issues:
I at least use these shortcuts a lot, but I guess that is not a strict requirement to have them fixed. It would just mean a lot of relearning for me. These both continue to not work (so they don't need to be fixed)
but this now works (before it didn't):
|
comment:28
Thank you. We are getting close.
works, but this does not (although it used to)
Although I could very easily argue that this is not supported behavior, so it might be more luck than anything it worked before. This is a very minor point and not something I would withhold a positive review for. |
comment:29
I do not think that we should allow options after the filenames. The usage is "options then filenames", and that's all. |
comment:31
Okay, then let's not (unofficially) support that input format anymore. Shouldn't Anyways, this should hopefully be the last of it. The sooner we can get this merged and out for people to test it more in the wild, the better we can pick up other changes in behavior (that people care about). |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:33
ok, short was indeed broken by the change of default value. Repaired now. Short was taking integers and will still take integers. I would prefer not to change that. |
comment:34
Okay, thank you. Let's get this tested by the rest of the developers. |
comment:35
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:37
damn. I have fixed a few things again. |
comment:38
Green bot and passes locally. |
Changed branch from u/chapoton/32332 to |
Changed commit from |
comment:40
This breaks running
|
comment:42
Nevermind, I see this is intentional (re comment 29) |
instead of the deprecated optparse
This was rather simple, but needs testing.
also full flake8 cleanup for the modified file
CC: @tscrim @slel @kliem @antonio-rojas
Component: scripts
Author: Frédéric Chapoton
Branch:
c87148b
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/32332
The text was updated successfully, but these errors were encountered: