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

Fix nested-min-max output msg #8234

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

clavedeluna
Copy link
Collaborator

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Fix nested-min-max suggestion message to indicate it's possible to splat iterable objects.

Closes #8168

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #8234 (a30dd4b) into main (6a019eb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8234   +/-   ##
=======================================
  Coverage   95.45%   95.45%           
=======================================
  Files         177      177           
  Lines       18646    18654    +8     
=======================================
+ Hits        17798    17806    +8     
  Misses        848      848           
Impacted Files Coverage Ξ”
pylint/checkers/nested_min_max.py 100.00% <100.00%> (ΓΈ)

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.

Looks amazing already !

@@ -1302,6 +1302,21 @@ def test_no_name_in_module(self) -> None:
[module, "-E"], expected_output="", unexpected_output=unexpected
)

def test_nested_min_max_splats(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this if we also have functional tests(I like functional tests a lot)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You'd think, but functional tests aren't testing the output message language exactly, so I felt better adding this to ensure "*" is in the message

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly there is an issue with * in functional tests ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No no, no issue. You can see the functional test .txt file looks great.

What I mean is that as far as I understand, the functional test will ensure that the message
nested-min-max appears in a specific line, indicated by adding # [nested-min-max] to that line

but what I want to test is not just that the msg is emitted, but that the msg suggestion specifically says "it's possible to do 'max(3, *nums, *lst2)' i". I can only assert this in the unit test, not the functional test.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/PyCQA/pylint/pull/8234/files#r1100266806 (Modify the expected output in this file and the tests should fail provided you have python 3.8 ore more recent)

@@ -238,6 +238,13 @@
{"_sitebuiltins.Quitter", "sys.exit", "posix._exit", "nt._exit"}
)

DICT_TYPES = (
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this in the variable checkers, we don't want to have checkers be coupled because of this constant and the module name 'utils' is horrible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fair point! the repo uses "utils" a ton. So just have both "DICT_TYPES" in each module regardless of duplication?

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit a30dd4b

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.

πŸ‘

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.0 milestone Feb 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Bug πŸͺ² backport maintenance/2.16.x and removed Enhancement ✨ Improvement to a component labels Feb 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.17.0, 2.16.2 Feb 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas merged commit 29684fe into pylint-dev:main Feb 8, 2023
github-actions bot pushed a commit that referenced this pull request Feb 8, 2023
Pierre-Sassoulas pushed a commit that referenced this pull request Feb 8, 2023
(cherry picked from commit 29684fe)

Co-authored-by: Dani Alcala <112832187+clavedeluna@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive nested-min-max
2 participants