osh optarg handling #2551
osh optarg handling #2551
Conversation
|
@andychu this seems to be a mycpp issue. |
|
Thanks for looking at this! Can you add a failing test first in Oh yeah and I guess this is the bug from
We didn't support that I will look at the C++ errors afterward ... this does appear to be a mycpp problem, since MyPy passes Not sure exactly why |
|
(And btw, I find this leading |
done |
|
Hm there are 4 failures in OSH Python , but only 3 are allowed https://op.oilshell.org/uuu/github-jobs/10775/ https://op.oilshell.org/uuu/github-jobs/10775/ovm-tarball.wwz/_tmp/spec/osh-py/builtin-getopts.html ( The |
There was a problem hiding this comment.
Hmm how about changing my_state.Fail() instead?
- Right now there 3 invocations of my_state.Fail(), which sets OPTARG to ''
- With this change there are 4 invocations
- but you only changed 2 of them?
Should the other 2 invocations also be changed?
If so, I think it is cleaner to change my_state.Fail() to have the if statement
And then the _GetOpts() code will look cleaner, with fewer if statements -- we may not need one every time that we need to Fail()
And then the behavior will also be consistent
I would start by trying to tickle the other error paths with tests
Are they not respecting leading : now?
spec/builtin-getopts.test.sh
Outdated
| err:? | ||
| ## END | ||
|
|
||
| #### getopts silent error reporting |
There was a problem hiding this comment.
Hm can you describe what this is testing? e.g. what is "silent error reporting" exactly?
Also should we assert STDERR does not contain an error message? I think it could be
## STDERR:
## END
which is different than if you don't have a leading : ?
I would like to see the two modes compared
from help getopts
If the first character of OPTSTRING is a colon, getopts uses silent error reporting.
In this mode, no error messages are printed.
If an invalid option is seen, getopts places the option character found into OPTARG.
OK good, we are testing this -- OPTARG=Z -- let's write a comment about this
If a required argument is not found, getopts places a ':' into NAME and sets OPTARG to the option character found.
Can we write a test for this too?
| self.spec_cache[spec_str] = spec | ||
| # OPTERR defaults to 1 | ||
| try: | ||
| opterr = state.GetInteger(self.mem, 'OPTERR') |
There was a problem hiding this comment.
Thanks for adding support for OPTERR
Can you also add a test for it?
builtin/pure_osh.py
Outdated
|
|
||
| # Hm doesn't cause status 1? | ||
| my_state.Fail() | ||
| if opterr != 0: |
There was a problem hiding this comment.
Let's add a comment that OPTERR=0 means that getopts disables error messages
( And also this is different than silent mode? That's weird ...)
|
Hm I also think it makes sense to put Then we can handle OPTERR in Fail() as well, rather than in (Or maybe we have to read OPTERR every time that Fail() is called? That is worth a test) |
| silent = spec_str.startswith(':') | ||
| if silent: | ||
| spec_str = spec_str[1:] | ||
| spec = _ParseOptSpec(spec_str) |
There was a problem hiding this comment.
Hm I also think we should preserve the spec_cache. Because getopts is often called in a loop like this
while getopts ':a:b:c:' flag; do
...
done
that prevents us from parsing the spec over and over again, on each iteration
So instead of Dict[str, bool], we could have a class, or we could use Zephyr ASDL
e.g. in core/runtime.asdl, it could be something like
GetOptsSpec = (Dict[str, bool] flags, bool silent)
And then we can cache that parsing in a dictionary self.spec_cache , e.g. Dict[str, GetOpsSpec]
I'm not sure if you've used Zephyr ASDL yet, but, it is a fairly important part of the codebase. It lets us define algebraic data types concisely
If you write out the same thing with a Python class, it is more verbose
Addresses this issue... #2547