Skip to content

Use trace not turtle in import_outside_toplevel test #7168

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 1 commit into from
Jul 12, 2022

Conversation

AdamWill
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Downstream (e.g. in Fedora) tkinter and turtle aren't always
included in the core Python package in order to avoid pulling in
a lot of graphical dependencies on systems that do not need them.
It doesn't really matter what libs are used in this test, so
let's replace it with trace.

Downstream (e.g. in Fedora) tkinter and turtle aren't always
included in the core Python package in order to avoid pulling in
a lot of graphical dependencies on systems that do not need them.
It doesn't really matter what libs are used in this test, so
let's replace it with trace.

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill
Copy link
Contributor Author

The Fedora package had the test disabled because of this failure (I guess the maintainer hadn't realized why the test was failing). We could just pull in our python-tkinter package as a build dependency, but I figured this was just as easy and doesn't bloat our build dependencies...

@jacobtylerwalls jacobtylerwalls added the Skip news πŸ”‡ This change does not require a changelog entry label Jul 12, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.15.0 milestone Jul 12, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2653368616

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.4%

Totals Coverage Status
Change from base Build 2650762732: 0.0%
Covered Lines: 16778
Relevant Lines: 17587

πŸ’› - Coveralls

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.

Make sense ! We had a similar report with solaris where apt was a builtin package. Sometime finding an import that work everywhere is not easy :)

@DanielNoord DanielNoord merged commit 9b5464d into pylint-dev:main Jul 12, 2022
@AdamWill
Copy link
Contributor Author

Yeah, I'm hoping trace should be good as I can't think of a reason to split it out, it's been in the stdlib forever, and it doesn't look in danger of removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants