Skip to content

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
🐛 Bug fix
✨ New feature

Description

Ref #5349 (comment).

@DanielNoord DanielNoord added Bug 🪲 Enhancement ✨ Improvement to a component labels Nov 24, 2021
@DanielNoord DanielNoord added this to the 2.12.0 milestone Nov 24, 2021
@coveralls
Copy link

coveralls commented Nov 24, 2021

Pull Request Test Coverage Report for Build 1502692283

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0004%) to 93.5%

Totals Coverage Status
Change from base Build 1500123956: 0.0004%
Covered Lines: 13996
Relevant Lines: 14969

💛 - 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.

Look good already, I have some minor remarks.


If you need special control over Pylint's configuration, you can also create a .rc file, which
can have sections of Pylint's configuration.
The .rc file can also contain a section ``[testoptions]`` to pass options for the functional
Copy link
Member

Choose a reason for hiding this comment

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

👌 This is very useful ! I wish I had that when starting out.

@Pierre-Sassoulas Pierre-Sassoulas merged commit fa7a84f into pylint-dev:main Nov 25, 2021
@DanielNoord DanielNoord deleted the min-pyver-end branch November 25, 2021 09:27
@DanielNoord DanielNoord mentioned this pull request Nov 25, 2021
9 tasks
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.

I didn't had much time the last few days. Just trying to catch up now.

min_pyver_end_position was exactly what I though off with my proposal. At the moment I agree that we don't need it. However there are still some AST node types left that don't have position information and just might get some in a future release.

Comment on lines +78 to +79
"min_pyver": Minimal python version required to run the test
"max_pyver": Maximum python version required to run the test
Copy link
Member

Choose a reason for hiding this comment

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

This isn't completely true. Whereas min_pyver acts inclusively, max_pyver is exclusive due to the way tuple comparisons and sys.version_info work in Python.

I.e. an alpha release would be (3, 11, 0, 'alpha', 0) > (3, 11). Thus setting max_pyver = 3.11 effectively excludes 3.11 and above.

IMO that is the desired behavior as otherwise one would need to say something like 3.10.99 to exclude 3.11+. So just the documentation that should get updated.

https://github.com/PyCQA/pylint/blob/1f7f2e96c39ee89ee9fb18ab23f3dfbb9325c870/pylint/testutils/lint_module_test.py#L92-L96

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @cdce8p for reviewing even after we already merged, much appreciated!

I'll create a PR that updates the doc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cdce8p Any good ideas about how to describe this. I have been trying for 5 minutes but can't come up with a simple but correct sentence 😆

Copy link
Member

Choose a reason for hiding this comment

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

"max_pyver": Starting from that python version for the test won't be run ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #5411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants