Conversation
|
This would also close #7008 |
5d0b5cd to
fd92022
Compare
There was a problem hiding this comment.
Thanks for the PR, it looks excellent!
I left a few minor comments, but the main thing to happen before merge is that we're confident in the API. What I posted in the issue was my initial idea, but upon playing around with your interpretation of it, a few questions have arisen.
First of all, I'm not sure it's desirable to interpret scope values as Typst code. I'd have expected the scope to always contain strings. Otherwise, it's not much different from just interpolating the variables into the main string. The idea was to offer a safe way to interpolate arbitrary strings. However, I'm not certain about this either. It feels somewhat redundant with sys.inputs. There have been various discussions about that one also supporting paths and bytes in some capacity (see #5382 (comment)) and those might crop up here, too. Since sys.inputs already works with typst eval out of the box, perhaps it's best to just rely on it and remove scope after all?
For mode, I'm also open whether it's actually pulling its weight then. (Should we remove scope, I don't feel quite as strong the urge for consistent with the eval function.)
What do you think?
crates/typst-cli/src/args.rs
Outdated
| Ok(StringInput::Stdin) | ||
| } else { | ||
| Ok(Input::Path(value.into())) | ||
| Ok(StringInput::String(value.to_string_lossy().into())) |
There was a problem hiding this comment.
I'm not sure we want to be lossy here. Is this the default behavior of clap when a string is required or does it error?
There was a problem hiding this comment.
Clap’s StringValueParser checks whether the input is valid UTF-8, and if it isn’t, it displays the command’s help/usage message. This means there’s no lossy conversion—invalid input results in an explicit error, which appears to be the standard for Clap.
I will use the NonEmptyStringValueParser instead of the OsStringValueParser. This should solve the issue
|
Thank you for the review and the ideas.
|
|
Thanks for the your detailed response. It's worth noting that the repetition could also be worked around like this: typst eval "{ let x = $(echo 2); x + x + 2 }"However, this easily makes for a code injection opportunity! I'm not sure what attack surface we're considering for This is less bad with scope, but I think it's still somewhat problematic and could be a pitfall. Something like Of course, you can avoid this by using Considering that
it might be a safer and better default. If you're expecting integers, I think it's also reasonable to be explicit about that and use There is also the question of whether |
|
Good point. I honestly did not think about potential attack surface on this one, but given your explanation, I agree that The repition mitigation with SummaryGiven your arguments:
|
I think (and that is how I implemented it for now) |
|
I'd say lets remove both |
|
Done |
Removes stdin from expression
|
Thanks a lot for the help along the way and the documentation/messages improvement from your side. |
|
Thank you, great work! |
Description
Deprecates
querysubcommandDisplay a deprecation warning when the
querysubcommand is invoked and also in clap help.No tests?
I have found no tests for individual CLI commands. So I decided to not design a new testsuite for raw CLI subcommands as that would be very much out of scope for this PR.
Closes #7344
Closes #7008