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

POETRY_EXPERIMENTAL_NEW_INSTALLER is interpreted as a boolean #3811

Conversation

AWhetter
Copy link
Contributor

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

The POETRY_EXPERIMENTAL_NEW_INSTALLER environment variable is not currently interpreted as a boolean value. This means that you have to set the value to an empty string to disable it.
This change adds the option to the list of options that are interpreted as booleans so that you can also disable it by setting it as anything other than "1" or "true".

@@ -140,6 +140,7 @@ def _get_normalizer(self, name: str) -> Callable:
"virtualenvs.create",
"virtualenvs.in-project",
"virtualenvs.options.always-copy",
"experimental.new-installer",
"installer.parallel",
}:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to make sure that this list is kept correct in the future by having the tests generate a complete list of options that should be interpreted as booleans and validating each one. However it would be just as valid to generate the list here (or do it at the class level so that it happens once) and then hardcoding a few values to check in the tests.
Doing it this way would mean that we could hand pick test cases that we know cover a lot of conditions and check that the environment variable names are correct . For example

@pytest.mark.parametrize(
    ("name", "env_var", "env_value", "value"),
    [
        ("installer.parallel", "POETRY_INSTALLER_PARALLEL", "true", True),
        ("installer.parallel", "POETRY_INSTALLER_PARALLEL", "false", False),
        ("virtualenvs.in-project", "POETRY_VIRTUALENVS_IN_PROJECT", "true", True),  # Using this poetry option makes sure that we replace both dots and dashes
        ("virtualenvs.in-project", "POETRY_VIRTUALENVS_IN_PROJECT", "false", False),
    ],
)
def test_config_get_from_environment_variable(...):
    ...

Let me know if you would prefer me to switch to using this approach instead.

@finswimmer finswimmer requested a review from a team March 20, 2021 09:14
abn
abn previously requested changes Mar 21, 2021
Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

Minor recommendation for tests.

tests/config/test_config.py Outdated Show resolved Hide resolved
@AWhetter AWhetter force-pushed the bugfix/experimental_new_installer_config_is_bool branch from 0c1e70d to 546d851 Compare March 21, 2021 17:38
@AWhetter
Copy link
Contributor Author

Thanks for the fast review! I've made the suggested changes.

@AWhetter AWhetter requested a review from abn March 24, 2021 23:16
br3ndonland added a commit to br3ndonland/fastenv that referenced this pull request Nov 1, 2021
python-poetry/poetry#3811
python-poetry/poetry#4210

Poetry installs are erroring with `JSONDecodeError`s in GitHub Actions:

```text
poetry install --no-interaction -E all

[debug]/usr/bin/bash -e /home/runner/work/_temp/0ba5f2f1-9726-40f5-9c09-24f1a8c4158a.sh

Creating virtualenv fastenv in /home/runner/work/fastenv/fastenv/.venv
Installing dependencies from lock file

Package operations: 53 installs, 0 updates, 0 removals

  • Installing six (1.16.0)

  JSONDecodeError

  Expecting value: line 1 column 1 (char 0)

at /opt/hostedtoolcache/Python/3.9.7/x64/lib/python3.9/json/decoder.py:355 in raw_decode
  351│         """
  352│         try:
  353│             obj, end = self.scan_once(s, idx)
  354│         except StopIteration as err:
→ 355│             raise JSONDecodeError("Expecting value", s, err.value) from None
  356│         return obj, end
  357│
```

`poetry cache clear --all pypi --no-interaction` does not fix the issue.
Removing the actions/cache step for Poetry fixes the issue.
Disabling the experimental new installer also fixes the issue.

Disable installer with `poetry config experimental.new-installer false`,
which updates poetry.toml.

Additionally, the setting is not correctly implemented as a Boolean, so
`export POETRY_EXPERIMENTAL_NEW_INSTALLER=false` does not disable the
installer (python-poetry/poetry#3811). It can be set to an empty string,
or disabled using `poetry config experimental.new-installer false` and
updating poetry.toml.

This commit will disable the experimental new installer in poetry.toml.
br3ndonland added a commit to br3ndonland/inboard that referenced this pull request Nov 2, 2021
python-poetry/poetry#3811
python-poetry/poetry#4210

Poetry installs are erroring with `JSONDecodeError`s in GitHub Actions:

```text
poetry install --no-interaction -E fastapi

[debug]/usr/bin/bash -e /home/runner/work/_temp/a14b9406-8aff-4354-80a9-ac90fef13c11.sh

Creating virtualenv inboard in /home/runner/work/inboard/inboard/.venv

Installing dependencies from lock file

Package operations: 53 installs, 0 updates, 0 removals

  • Installing six (1.16.0)

  JSONDecodeError

  Expecting value: line 1 column 1 (char 0)

at /opt/hostedtoolcache/Python/3.9.7/x64/lib/python3.9/json/decoder.py:355 in raw_decode
  351│         """
  352│         try:
  353│             obj, end = self.scan_once(s, idx)
  354│         except StopIteration as err:
→ 355│             raise JSONDecodeError("Expecting value", s, err.value) from None
  356│         return obj, end
  357│
```

`poetry cache clear --all pypi --no-interaction` does not fix the issue.
Removing the actions/cache step for Poetry fixes the issue.
Disabling the experimental new installer also fixes the issue.

Disable installer with `poetry config experimental.new-installer false`,
which updates poetry.toml.

Additionally, the setting is not correctly implemented as a Boolean, so
`export POETRY_EXPERIMENTAL_NEW_INSTALLER=false` does not disable the
installer (python-poetry/poetry#3811). It can be set to an empty string,
or disabled using `poetry config experimental.new-installer false` and
updating poetry.toml.

This commit will disable the experimental new installer in poetry.toml.
br3ndonland added a commit to br3ndonland/template-python that referenced this pull request Nov 2, 2021
python-poetry/poetry#3811
python-poetry/poetry#4210

Poetry installs are erroring with `JSONDecodeError`s in GitHub Actions:

`poetry cache clear --all pypi --no-interaction` does not fix the issue.
Removing the actions/cache step for Poetry fixes the issue.
Disabling the experimental new installer also fixes the issue.

Disable installer with `poetry config experimental.new-installer false`,
which updates poetry.toml.

Additionally, the setting is not correctly implemented as a Boolean, so
`export POETRY_EXPERIMENTAL_NEW_INSTALLER=false` does not disable the
installer (python-poetry/poetry#3811). It can be set to an empty string,
or disabled using `poetry config experimental.new-installer false` and
updating poetry.toml.

This commit will disable the experimental new installer in poetry.toml.
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Both the fix and the test changes look good. I think that ultimately your approach of using a generator is the better, even if it adds a bit to the mental overhead when maintaining the test.

Could you please break up your changes into two comments with good commit messages? LGTM once that's done.

@neersighted neersighted self-assigned this Nov 14, 2021
@neersighted neersighted force-pushed the bugfix/experimental_new_installer_config_is_bool branch from 546d851 to e073131 Compare November 15, 2021 01:53
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Opted to merge as-is.

@neersighted neersighted merged commit d4c2fe4 into python-poetry:master Nov 15, 2021
@abn abn mentioned this pull request Mar 16, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
@AWhetter AWhetter deleted the bugfix/experimental_new_installer_config_is_bool branch March 1, 2024 05:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants