Skip to content
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

feat(cli) Improve environment variables parsing #2042

Merged
merged 4 commits into from
Jan 22, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jan 22, 2021

Description

This PR fixes #2028. In #1762 we have already improved environment variables syntax, but the same work must be done in the CLI crate.

The following environment variables are considered incorrect:

  • A, no equal sign,
  • A=, value is missing,
  • =A, key is missing.

The following environment variables are considered correct:

  • A=B with A for the name and B for the value,
  • A=B=C with A for the name and B=C for the value.

The environment variable is trimmed before being parsed with str::trim.

This PR also adds tests for the parse_envvar function, which was missing.

Review

  • Add a short description of the the change to the CHANGELOG.md file

The following environment variables are considered incorrect:

* `A`, no equal sign,
* `A=`, value is missing,
* `=A`, key is missing.

The following environment variables are considered correct:

* `A=B` with `A` for the name and `B` for the value,
* `A=B=C` with `A` for the name and `B=C` for the value.
@Hywan Hywan added 🎉 enhancement New feature! 🧪 tests I love tests 📦 lib-cli About wasmer-cli labels Jan 22, 2021
@Hywan Hywan self-assigned this Jan 22, 2021
@Hywan Hywan requested a review from syrusakbary as a code owner January 22, 2021 12:54
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks good , thanks for the tests!

&entry
),

Some(position) => Ok((entry[..position].into(), entry[position + 1..].into())),
Copy link
Contributor

Choose a reason for hiding this comment

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

The actual splitting can just be done with just https://doc.rust-lang.org/std/primitive.str.html#method.split_at ; but we won't have as many error cases to report.

Just a heads up, I'm fine keeping it as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to deal with empty string for the left- or right-end side of the =. I think the code would not be simplified that much. Thanks for the heads-up!

@Hywan
Copy link
Contributor Author

Hywan commented Jan 22, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 22, 2021

@bors bors bot merged commit b7e1987 into wasmerio:master Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-cli About wasmer-cli 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: --env should allow variables that contain =
2 participants