std/cmdline.commandLineParams NimScript fix#25573
std/cmdline.commandLineParams NimScript fix#25573ZoomRmc wants to merge 2 commits intonim-lang:develfrom
std/cmdline.commandLineParams NimScript fix#25573Conversation
| result = @[] | ||
| for i in 1..paramCount(): | ||
| result.add(paramStr(i)) | ||
| when defined(nimscript): |
There was a problem hiding this comment.
The compiler as it evaluates paramCount and paramStr at compile-time should do these fixups.
There was a problem hiding this comment.
Well, maybe it should but it doesn't. These procs are builtins for NimScript, as far as I see. Not sure what you want.
This code is not new, it's just moved copied from parseopt to a more fitting place.
There was a problem hiding this comment.
I want you to fix the root cause which is in compiler/scriptconfig.nim, lines:
cbconf paramStr:
setResult(a, os.paramStr(int a.getInt 0))
cbconf paramCount:
setResult(a, os.paramCount())
There was a problem hiding this comment.
First of all, I wouldn't know how to, I found the source but I'm not very confident at that level.
Secondly, wouldn't this break scripts inspecting the command line launched implicitly from config.nims on compilation?
# in config.nims:
for i in 1..paramCount():
if paramStr(i) == "-foo":
raise newException(Defect, "Build with 'foo' is not supported")|
So, continuing the discussion above. As we have an additional case of implicitly run scripts, I think it would be beneficial to leave the The |
|
@Araq I'd like to proceed with this, but I think my concerns above are legit. To repeat myself, two things need clarifications:
|
Now consistently returns only the script arguments, skipping compiler arguments.
- Documents and codifies behavioral equivalence of `cmdline.commandLineParams` and `parseopt.initOptParser`. - Documents `paramStr` and `paramCount` being nimscript-agnostic. - Tests commandLineParams and parseopt do not return nim arguments when the script is invoked implicitly by compiler and explicitly.
|
Rebased on tip, tests added. I went with "nothing" as the answer to my second question in the previous post. It's documented and you still have the choice of what to use. Being smarter requires the compiler to expose how the script was invoked in some way. I think this should be good to go. |
cmdline.commandLineParams()now returns only the script arguments, skipping compiler arguments.Closes #23554
The logic is extracted from
parseopt.initOptParserand is now duplicated. Though there it triggers only when there are no arguments provided so double reads are an edge case:Read
commandLineParams()from a NimScript and get none -> pass them explicitly toinitOptParser-> Whole argv reread again and stripped.Should be fixed if #25571 or an alternative is merged.