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

build: switch to tomli for TOML v1 compliant parser #336

Merged
merged 1 commit into from Aug 2, 2021
Merged

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Aug 1, 2021

Fixes #308

Signed-off-by: Filipe Laíns lains@riseup.net

@layday
Copy link
Member

layday commented Aug 1, 2021

  • Reading from a text stream is deprecated in tomli and will generate a warning which can be reinterpreted as an error in test suites.
  • Please replace toml with tomli in tests/constraints.txt.
  • Remove types-toml from setup.cfg.

Comment on lines 25 to 33
try:
import tomli as toml

TOMLDecodeError: Type[Exception] = toml.TOMLDecodeError
except ModuleNotFoundError: # pragma: no cover
import toml as _toml

toml = _toml # type: ignore
TOMLDecodeError = _toml.TomlDecodeError
Copy link

Choose a reason for hiding this comment

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

Suggested change
try:
import tomli as toml
TOMLDecodeError: Type[Exception] = toml.TOMLDecodeError
except ModuleNotFoundError: # pragma: no cover
import toml as _toml
toml = _toml # type: ignore
TOMLDecodeError = _toml.TomlDecodeError
try:
from tomli import loads as toml_loads
from tomli import TOMLDecodeError
except ImportError: # pragma: no cover
from toml import loads as toml_loads
from toml import TomlDecodeError as TOMLDecodeError

Something like this plus changing these lines below

with open(spec_file, encoding='UTF-8') as f:
    spec = toml.load(f)

to

with open(spec_file, 'rb') as f:
        spec = toml_loads(f.read().decode())

will

  1. avoid a deprecation warning in tomli
  2. avoid making illegal line ending conversions when opening the file

Copy link
Member Author

Choose a reason for hiding this comment

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

I generally prefer to keep full imports, but since we were already doing it for the error, we can have loads too.

@FFY00 FFY00 force-pushed the toml1 branch 2 times, most recently from 22cf852 to 153eecb Compare August 1, 2021 23:59
src/build/__init__.py Show resolved Hide resolved
def mock_tomli_not_available(mocker):
loads = mocker.patch('tomli.loads')
tomli = sys.modules['tomli']
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you want

patch.dict(sys.modules, {'tomli': None})

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I did not know about it 😛

@FFY00 FFY00 force-pushed the toml1 branch 3 times, most recently from 993bf1f to ca4814d Compare August 2, 2021 15:46
@FFY00
Copy link
Member Author

FFY00 commented Aug 2, 2021

Of course 🤦

@FFY00 FFY00 requested a review from gaborbernat August 2, 2021 15:57
@FFY00
Copy link
Member Author

FFY00 commented Aug 2, 2021

Nevermind, the teardown does not run after I yield or return the other fixture. Does anyone have any proposal on how to do this cleanly? pytest-mock is very much against giving us access over when the mock is removed, we would use unittest.mock directly.

@gaborbernat
Copy link
Contributor

pytest-mock is very much against giving us access over when the mock is removed, we would use unittest.mock directly.

Yeah in this case I'd import and use that directly 🤔

@layday
Copy link
Member

layday commented Aug 2, 2021

You are reloading the module before sys.modules is unpatched.

@layday
Copy link
Member

layday commented Aug 2, 2021

Okay, I tried running the tests locally and I think I see what's going on. Try calling stopall on the mocker:

@pytest.fixture
def mock_tomli_not_available(mocker):
    loads = mocker.patch('tomli.loads')
    mocker.patch.dict(sys.modules, {'tomli': None})
    importlib.reload(build)
    try:
        yield
    finally:
        loads.assert_not_called()
        mocker.stopall()
        importlib.reload(build)

Fixes pypa#308

Co-authored-by: layday <layday@protonmail.com>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Aug 2, 2021

That works too, and our use-case is simple enough that using stopall is fine.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@FFY00
Copy link
Member Author

FFY00 commented Aug 2, 2021

😊

@FFY00 FFY00 merged commit 612dde4 into pypa:main Aug 2, 2021
@FFY00 FFY00 deleted the toml1 branch August 2, 2021 20:42
inmantaci pushed a commit to inmanta/inmanta-core that referenced this pull request Aug 6, 2021
Bumps [build](https://github.com/pypa/build) from 0.5.1 to 0.6.0.post1.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/pypa/build/blob/main/CHANGELOG.rst">build's changelog</a>.</em></p>
<blockquote>
<h1>0.6.0.post1 (05-08-2021)</h1>
<ul>
<li>Fix compability with Python 3.6 and 3.7 (<code>PR [#339](https://github.com/pypa/build/issues/339)</code><em>, Fixes <code>[#338](https://github.com/pypa/build/issues/338)</code></em>)</li>
</ul>
<p>.. _PR <a href="https://github-redirect.dependabot.com/pypa/build/issues/339">#339</a>: <a href="https://github-redirect.dependabot.com/pypa/build/pull/339">pypa/build#339</a>
.. _<a href="https://github-redirect.dependabot.com/pypa/build/issues/338">#338</a>: <a href="https://github-redirect.dependabot.com/pypa/build/issues/338">pypa/build#338</a></p>
<h1>0.6.0 (02-08-2021)</h1>
<ul>
<li>Improved output (<code>PR [#333](https://github.com/pypa/build/issues/333)</code><em>, Fixes <code>[#142](https://github.com/pypa/build/issues/142)</code></em>)</li>
<li>The CLI now honnors <code>NO_COLOR</code>_ (<code>PR [#333](https://github.com/pypa/build/issues/333)</code>_)</li>
<li>The CLI can now be forced to colorize the output by setting the <code>FORCE_COLOR</code> environment variable (<code>PR [#335](https://github.com/pypa/build/issues/335)</code>_)</li>
<li>Added logging to <code>build</code> and <code>build.env</code> (<code>PR [#333](https://github.com/pypa/build/issues/333)</code>_)</li>
<li>Switch to a TOML v1 compliant parser (<code>PR [#336](https://github.com/pypa/build/issues/336)</code><em>, Fixes <code>[#308](https://github.com/pypa/build/issues/308)</code></em>)</li>
</ul>
<h2>Breaking Changes</h2>
<ul>
<li>Dropped support for Python 2 and 3.5.</li>
</ul>
<p>.. _PR <a href="https://github-redirect.dependabot.com/pypa/build/issues/333">#333</a>: <a href="https://github-redirect.dependabot.com/pypa/build/pull/333">pypa/build#333</a>
.. _PR <a href="https://github-redirect.dependabot.com/pypa/build/issues/335">#335</a>: <a href="https://github-redirect.dependabot.com/pypa/build/pull/335">pypa/build#335</a>
.. _PR <a href="https://github-redirect.dependabot.com/pypa/build/issues/336">#336</a>: <a href="https://github-redirect.dependabot.com/pypa/build/pull/336">pypa/build#336</a>
.. _<a href="https://github-redirect.dependabot.com/pypa/build/issues/142">#142</a>: <a href="https://github-redirect.dependabot.com/pypa/build/issues/142">pypa/build#142</a>
.. _<a href="https://github-redirect.dependabot.com/pypa/build/issues/308">#308</a>: <a href="https://github-redirect.dependabot.com/pypa/build/issues/308">pypa/build#308</a>
.. _NO_COLOR: <a href="https://no-color.org">https://no-color.org</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/pypa/build/commit/018a6f81cca1b26d9bf754eb91a64667c3847d28"><code>018a6f8</code></a> release 0.6.0.post1</li>
<li><a href="https://github.com/pypa/build/commit/84412dbbb29f08fdc0040ce75c81107843f7c496"><code>84412db</code></a> changelog: add Python 3.6 and 3.7 compability fix</li>
<li><a href="https://github.com/pypa/build/commit/c3f9e6d8c11c6ca3f71159a0347de171395cddd9"><code>c3f9e6d</code></a> build: restore compatibility with Python 3.6 and 3.7</li>
<li><a href="https://github.com/pypa/build/commit/720d69dadc1641769be89e3630b12b207e157ca3"><code>720d69d</code></a> tests: enable pytest strict mode</li>
<li><a href="https://github.com/pypa/build/commit/4cb0c1ec375aeff3e681855fdb37cf8f47e0fa93"><code>4cb0c1e</code></a> setup.cfg: fix typo in version</li>
<li><a href="https://github.com/pypa/build/commit/48c68979d95f070c093125b663dbd074399b5619"><code>48c6897</code></a> release 0.6.0</li>
<li><a href="https://github.com/pypa/build/commit/612dde4a4d538d1f0547ee524667b11534cba2cd"><code>612dde4</code></a> build: switch to tomli for TOML v1 compliant parser</li>
<li><a href="https://github.com/pypa/build/commit/7ed0a6bf06e05e6ccc72a99fe8194ebb9a2f1d20"><code>7ed0a6b</code></a> main: add line between backend output and missing dependencies error</li>
<li><a href="https://github.com/pypa/build/commit/79e777452cf3a75392b858f5ec41fc41eb4403b7"><code>79e7774</code></a> main: refactor color detection</li>
<li><a href="https://github.com/pypa/build/commit/03ad5d4af2cb23c13ab2de4672c359afbb99ab6b"><code>03ad5d4</code></a> main: add FORCE_COLOR</li>
<li>Additional commits viewable in <a href="https://github.com/pypa/build/compare/0.5.1...0.6.0.post1">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=build&package-manager=pip&previous-version=0.5.1&new-version=0.6.0.post1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

</details>
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.

Consider switching to TOML v1 compliant parser
4 participants