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

Update to scie jump 0.13.2, for errors on invalid .env #319

Closed
wants to merge 4 commits into from

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Nov 3, 2023

This fixes #307 by ensuring that an invalid .env file is flagged loudly, rather than silently ignored. This is done by updating scie jump to pick up a-scie/jump#163 / a-scie/jump#164 in https://github.com/a-scie/jump/releases/tag/v0.13.2 .

This is technically a breaking change, as someone may be using scie-pants fine, despite having an .env file lying around that's not understood by the underlying dotenvy. Thus, this is a bump to 0.11, rather than a patch to 0.10.

@huonw huonw marked this pull request as ready for review November 3, 2023 05:18
@huonw huonw requested a review from kaos November 3, 2023 05:18
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

lovely

CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Andreas Stenius <git@astekk.se>
huonw added a commit to pantsbuild/pants that referenced this pull request 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 pull request 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 pull request 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 pull request 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 <huon@exoflare.io>
huonw added a commit to pantsbuild/pants that referenced this pull request 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 <huon@exoflare.io>
@huonw
Copy link
Contributor Author

huonw commented Nov 4, 2023

Note to self: I've reconsidered and think we shouldn't merge this as is, and instead make it less breaking before merging. Potentially both of:

  • have this be a warning first, before being an error
  • wait for support for at least X="double quotes", since that's probably fairly common

@@ -517,6 +518,26 @@ fn test_dot_env_loading(scie_pants_scie: &Path, clone_root: &TempDir) {
.unwrap();
}

fn test_dot_env_error(scie_pants_scie: &Path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what to make of exchanges like this: #339 (comment)

That PR got submitted trampling this test. The test seemed like a great idea to me.

@huonw
Copy link
Contributor Author

huonw commented Apr 22, 2024

Replaced by #339 and now #389

@huonw huonw closed this Apr 22, 2024
@huonw huonw deleted the huonw/scie-jump branch April 22, 2024 02:03
huonw added a commit that referenced this pull request Apr 25, 2024
This adds a test to ensure that an invalid `.env` file is detected.

(Replacing #319 after #339.)
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.

.env loading silently fails under common scenarios
3 participants