Skip to content

Update toml dependency to >=0.9.2 #5067

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

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

jeroenseegers
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Fix the minimum version of the toml dependency as described in #5066.

Closes #5066

@coveralls
Copy link

coveralls commented Sep 23, 2021

Pull Request Test Coverage Report for Build 1266038107

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.091%

Totals Coverage Status
Change from base Build 1265024268: 0.0%
Covered Lines: 13352
Relevant Lines: 14343

πŸ’› - Coveralls

@Pierre-Sassoulas
Copy link
Member

I came across this due to a shared dependency on toml between two packages, of which one has toml<0.10,>=0.9 causing toml==0.9.6 to be installed.

Wouldn't that make pylint incompatible with this package ? Maybe the fix is to handle both import by adding an import guard and keep the compatibility with toml > 0.7 ?

@Pierre-Sassoulas Pierre-Sassoulas added the Crash πŸ’₯ A bug that makes pylint crash label Sep 23, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.11.2 milestone Sep 23, 2021
@jeroenseegers
Copy link
Contributor Author

jeroenseegers commented Sep 23, 2021

I came across this due to a shared dependency on toml between two packages, of which one has toml<0.10,>=0.9 causing toml==0.9.6 to be installed.

Wouldn't that make pylint incompatible with this package ? Maybe the fix is to handle both import by adding an import guard and keep the compatibility with toml > 0.7 ?

I'm not sure if that's very relevant in this case. It's a tag of a package > 2 years old by now (0), and this particular bug has been present for ~7 months in pylint without any previous mention of it before this issue and PR.

@cdce8p
Copy link
Member

cdce8p commented Sep 23, 2021

From what I can tell, we should be able to import TomlDecodeError from toml directly even in 0.10.0. Just need to update the code. With that we shouldn't need to update the dependency.
https://github.com/PyCQA/pylint/blob/09a0b50bfeb473625f8ad586f72fd35d790de6ac/pylint/config/find_default_config_files.py#L8

toml/__init__.py - 0.10.0

@jeroenseegers
Copy link
Contributor Author

jeroenseegers commented Sep 23, 2021

From what I can tell, we should be able to import TomlDecodeError from toml directly even in 0.10.0. Just need to update the code. With that we shouldn't need to update the dependency.

https://github.com/PyCQA/pylint/blob/09a0b50bfeb473625f8ad586f72fd35d790de6ac/pylint/config/find_default_config_files.py#L8

toml/__init__.py - 0.10.0

The issue is that this import is only available from 0.10.0 onwards, not in earlier versions.

From 0.9.2 it's available as from toml import TomlDecodeError: https://github.com/uiri/toml/blob/0.9.2/toml.py

Before that, TomlDecodeError does not exist at all.

@cdce8p
Copy link
Member

cdce8p commented Sep 23, 2021

The issue is that this import is only available from 0.10.0 onwards, not in earlier versions.

From 0.9.2 it's available as from toml import TomlDecodeError`: https://github.com/uiri/toml/blob/0.9.2/toml.py

Before that, TomlDecodeError does not exist at all.

πŸ‘πŸ» In that case we should probably require 0.9.2.

@jeroenseegers
Copy link
Contributor Author

πŸ‘πŸ» In that case we should probably require 0.9.2.

Shall I then make it toml>=0.9.2,<0.10 and change
https://github.com/PyCQA/pylint/blob/09a0b50bfeb473625f8ad586f72fd35d790de6ac/pylint/config/find_default_config_files.py#L8
to

from toml import TomlDecodeError

?

@jeroenseegers jeroenseegers changed the title Update toml dependency from >=0.7.1 to >=0.10.0 Update toml dependency from >=0.7.1 to >=0.9.2 Sep 23, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @jeroenseegers 🐬

@cdce8p cdce8p changed the title Update toml dependency from >=0.7.1 to >=0.9.2 Update toml dependency to >=0.9.2 Sep 23, 2021
@cdce8p cdce8p added the dependency Label for github dependabot label Sep 23, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you, great catch !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 9dc186f into pylint-dev:main Sep 23, 2021
@jeroenseegers jeroenseegers deleted the toml-dependency branch September 24, 2021 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Crash πŸ’₯ A bug that makes pylint crash dependency Label for github dependabot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TOML dependency declaration is outdated
5 participants