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

Consider supporting VAR="with space and $substitution" in .env #166

Closed
huonw opened this issue Nov 3, 2023 · 6 comments · Fixed by #180
Closed

Consider supporting VAR="with space and $substitution" in .env #166

huonw opened this issue Nov 3, 2023 · 6 comments · Fixed by #180
Assignees
Labels
enhancement New feature or request

Comments

@huonw
Copy link
Collaborator

huonw commented Nov 3, 2023

Continuing #163, it seems that double quotes aren't supported, so it is inconvenient to use .env to set a variable that has both a space in it and does a $ env var substitution, because all the spaces have to be individually \ escaped.

In particular, it'd be nice to have this table have 4 ✅:

.env line Behaviour Behaviour is desirable Comment
A="x $y" Error This is the most natural syntax, and would be nice
A=x\ $y Accepted, with substitution Annoying for a human to have to write and read all the \s
A='x $y' Accepted, without substitution matches the same line in a shell
A=x $y Error if accepted, this would have different meaning to the same line in a shell

This is effectively a limitation of dotenvy, e.g. allan2/dotenvy#11. I don't know if another library has different behaviour.

@jsirois
Copy link
Collaborator

jsirois commented Nov 3, 2023

@huonw presumably you've read through the dotenvy issue; so you realize you're asking scie jump to roll it's own internally or else create a new .env handling crate. The root issue here is that .env is not standardized, but it is prevalent. You're fully aware of the xkcd-worthy traps of either path. Did you have a choice in mind?

@huonw
Copy link
Collaborator Author

huonw commented Nov 3, 2023

Yes, I'm aware that the reality of actually improving this particular feature may make it not worth it (or, maybe just left for some motivated contributor to improve dotenvy first). In any case, dotenvy is an implementation detail of jump, and a consumer like scie-pants doesn't get any contro (e.g. it can't swap it out) so it seems squarely jump's place to consider the .env files it'll accept, even if it's just "whatever dotenvy accepts is fine".

In terms of alternatives, https://github.com/arniu/dotenvs-rs seems to be a little more explicit about what syntax it accepts... but hits arniu/dotenvs-rs#4 (i.e. #163 again, which is more important than this one) so... 🤷‍♂️

Thus, I have no concrete path forward other than either roll-ones-own (bleh), or improving dotenvy (more attractive)... and of course no expectation that you specifically will do either or even keep this issue open 😄

@jsirois
Copy link
Collaborator

jsirois commented Nov 3, 2023

Ok, thanks for a fuller picture of the request. To complete the picture, afaict the motivating use case is a script that generates a .env file trying to handle source root paths with spaces in them being complicated by the lack of support for using double quotes with interpolation; thus requiring escaping instead. This is a case that is unlikely to exist, but may, and thus is necessary to treat.

Further, the .env file in question is not for use by the scie itself or anything it runs, it merely needs to be parseable so as to not kill the scie. The file will in fact be parsed and used by another program altogether (VSCode).

@jsirois
Copy link
Collaborator

jsirois commented Nov 3, 2023

Documenting the supported .env format is, of course, the right thing to do no matter the outcome here (#167).

huonw added a commit to pantsbuild/pants that referenced this issue Nov 3, 2023
This fixes #20127 by avoiding double quotes in the `.env` file generated
by the code snippet in the IDE set-up docs.

Double quotes aren't supported by the `scie-pants` `.env` loader, and a
line containing them was previously silently ignored (plus all lines
after), but with scie-pants 0.11.0, will be an error. See
pantsbuild/scie-pants#307 and
pantsbuild/scie-pants#319.

The new suggestion has _no_ quotes at all, because there's no other way
to have a line that includes a substitution. To handle source roots with
spaces in their paths, this has to also manually escape them. Supporting
double quotes is effectively a new feature of `scie-pants` launcher (via
scie jump a-scie/jump#166 via `dotenvy`
allan2/dotenvy#11), but for now we can at
least stop suggesting incorrect things.

For instance, with source roots `a b/c` and `def`, this generates:

```
PYTHONPATH=./a\ b/c:./def:$PYTHONPATH
```
WorkerPants pushed a commit to pantsbuild/pants that referenced this issue Nov 3, 2023
This fixes #20127 by avoiding double quotes in the `.env` file generated
by the code snippet in the IDE set-up docs.

Double quotes aren't supported by the `scie-pants` `.env` loader, and a
line containing them was previously silently ignored (plus all lines
after), but with scie-pants 0.11.0, will be an error. See
pantsbuild/scie-pants#307 and
pantsbuild/scie-pants#319.

The new suggestion has _no_ quotes at all, because there's no other way
to have a line that includes a substitution. To handle source roots with
spaces in their paths, this has to also manually escape them. Supporting
double quotes is effectively a new feature of `scie-pants` launcher (via
scie jump a-scie/jump#166 via `dotenvy`
allan2/dotenvy#11), but for now we can at
least stop suggesting incorrect things.

For instance, with source roots `a b/c` and `def`, this generates:

```
PYTHONPATH=./a\ b/c:./def:$PYTHONPATH
```
WorkerPants pushed a commit to pantsbuild/pants that referenced this issue Nov 3, 2023
This fixes #20127 by avoiding double quotes in the `.env` file generated
by the code snippet in the IDE set-up docs.

Double quotes aren't supported by the `scie-pants` `.env` loader, and a
line containing them was previously silently ignored (plus all lines
after), but with scie-pants 0.11.0, will be an error. See
pantsbuild/scie-pants#307 and
pantsbuild/scie-pants#319.

The new suggestion has _no_ quotes at all, because there's no other way
to have a line that includes a substitution. To handle source roots with
spaces in their paths, this has to also manually escape them. Supporting
double quotes is effectively a new feature of `scie-pants` launcher (via
scie jump a-scie/jump#166 via `dotenvy`
allan2/dotenvy#11), but for now we can at
least stop suggesting incorrect things.

For instance, with source roots `a b/c` and `def`, this generates:

```
PYTHONPATH=./a\ b/c:./def:$PYTHONPATH
```
huonw added a commit to pantsbuild/pants that referenced this issue Nov 3, 2023
…20146)

This fixes #20127 by avoiding double quotes in the `.env` file generated
by the code snippet in the IDE set-up docs.

Double quotes aren't supported by the `scie-pants` `.env` loader, and a
line containing them was previously silently ignored (plus all lines
after), but with scie-pants 0.11.0, will be an error. See
pantsbuild/scie-pants#307 and
pantsbuild/scie-pants#319.

The new suggestion has _no_ quotes at all, because there's no other way
to have a line that includes a substitution. To handle source roots with
spaces in their paths, this has to also manually escape them. Supporting
double quotes is effectively a new feature of `scie-pants` launcher (via
scie jump a-scie/jump#166 via `dotenvy`
allan2/dotenvy#11), but for now we can at
least stop suggesting incorrect things.

For instance, with source roots `a b/c` and `def`, this generates:

```
PYTHONPATH=./a\ b/c:./def:$PYTHONPATH
```

Co-authored-by: Huon Wilson <[email protected]>
huonw added a commit to pantsbuild/pants that referenced this issue Nov 3, 2023
…20145)

This fixes #20127 by avoiding double quotes in the `.env` file generated
by the code snippet in the IDE set-up docs.

Double quotes aren't supported by the `scie-pants` `.env` loader, and a
line containing them was previously silently ignored (plus all lines
after), but with scie-pants 0.11.0, will be an error. See
pantsbuild/scie-pants#307 and
pantsbuild/scie-pants#319.

The new suggestion has _no_ quotes at all, because there's no other way
to have a line that includes a substitution. To handle source roots with
spaces in their paths, this has to also manually escape them. Supporting
double quotes is effectively a new feature of `scie-pants` launcher (via
scie jump a-scie/jump#166 via `dotenvy`
allan2/dotenvy#11), but for now we can at
least stop suggesting incorrect things.

For instance, with source roots `a b/c` and `def`, this generates:

```
PYTHONPATH=./a\ b/c:./def:$PYTHONPATH
```

Co-authored-by: Huon Wilson <[email protected]>
@jsirois
Copy link
Collaborator

jsirois commented Nov 4, 2023

Alright - dotenvs-rs LGTM; I gave it a go: arniu/dotenvs-rs#5 - we'll see if that gets accepted.

@jsirois jsirois self-assigned this Dec 22, 2023
@jsirois jsirois added the in progress Indicates the assignee is actively working on the item. label Dec 22, 2023
@jsirois
Copy link
Collaborator

jsirois commented Dec 22, 2023

You won't get item 4 (A=x $y errors), but switching to dotenvs-rs gets you the 1st 3. I think I feel fine documenting this and not policing the shell on its behalf. If A user wants to source their env in the shell, they can let the shell tell them the syntax is bad for it.

jsirois added a commit to jsirois/jump that referenced this issue Dec 22, 2023
Switch from the dotenvy crate to dotenvs to achieve this. A patched
version is used to ensure we maintain support for hard failure when
invalid `.env` files are read.

Fixes a-scie#166
Fixes a-scie#167
jsirois added a commit to jsirois/jump that referenced this issue Dec 22, 2023
Switch from the dotenvy crate to dotenvs to achieve this. A patched
version is used to ensure we maintain support for hard failure when
invalid `.env` files are read.

Fixes a-scie#166
Fixes a-scie#167
jsirois added a commit that referenced this issue Dec 22, 2023
Switch from the dotenvy crate to dotenvs to achieve this. A patched
version is used to ensure we maintain support for hard failure when
invalid `.env` files are read.

Fixes #166
Fixes #167
@jsirois jsirois removed the in progress Indicates the assignee is actively working on the item. label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants