Skip to content

fix: change command parser to not trim whitespaces #10

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

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

williamboman
Copy link
Contributor

@williamboman williamboman commented Aug 22, 2022

This is to allow for commands where passing an empty string as argument
has significance. This should however only be relevant in exceptional
cases. For example, shellharden handles reading from stdin by receiving
an empty string as its "file" argument (in shell done via ''/""):

$ shellharden --transform '' < file

A translation to Rust std::process style of spawning processes would be:
cmd: "shellharden"
args: ["--transform", ""]

By using .split_whitespace() to separate the components of a command,
any trailing as well as contiguous sequences of whitespaces are removed,
making it impossible to express the same command via the TOML config:

[languages]
sh = ["shellharden --transform "]

This PR changes the command parser to treat whitespace individually, and
not trim any away. I guess one drawback of this is that the commands
expressed in the TOML config drift further away from what one perhaps
would expect. By this I mean a shell-like syntax where whitespace don't
matter as much (until they do, by which time you're in for a world of
pain).

I wouldn't mind if this was declined due to similar reasoning, figured
I'd open it anyway and at least highlight the problem. In that case I'll
try to get - supported as a filename representing stdin in
shellharden, this PR would be a good motivation for it :)

This is to allow for commands where passing an empty string as argument
has significance. This should however only be relevant in exceptional
cases. For example, shellharden handles reading from stdin by receiving
an empty string as its "file" argument (in shell, done via ''):

```sh
$ shellharden --transform '' < file
```

A translation to Rust std::process style of spawning processes would be:
cmd:   "shellharden"
args: ["--transform", ""]

By using `.split_whitespace()` to separate the components of a command,
any trailing as well as contiguous sequences of whitespaces are removed,
making it impossible to express the same command via the TOML config:

```toml
[languages]
sh = ["shellharden --transform "]
```

This PR changes the command parser to treat whitespace individually, and
not trim any away. I guess one drawback of this is that the commands
expressed in the TOML config drift further away from what one perhaps
would expect. By this I mean a shell-like syntax where whitespace don't
matter as much (until they do, by which time you're in for a world of
pain).

I wouldn't mind if this was declined due to similar reasoning, figured
I'd open it anyway and at least highlight the problem. In that case I'll
try to get `-` supported as a filename representing stdin in
`shellharden`, this PR would be a good motivation for it :)
Copy link
Owner

@lukas-reineke lukas-reineke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I made the command a list directly in toml, but decided to turn it into a string because I thought that would be easier.
Maybe that wasn't the best decision, shell can be weird..

I think the PR makes sense, though. Just ignoring empty strings is not what I would expect as a user. I'm fine with merging this, thanks

@lukas-reineke lukas-reineke merged commit 70ee804 into lukas-reineke:master Aug 23, 2022
@williamboman williamboman deleted the fix/command-parser branch August 25, 2022 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants