Skip to content

Conversation

tusharsadhwani
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Fixes pylint-dev/pylint#8544

@tusharsadhwani
Copy link
Contributor Author

This is causing one test for me to fail locally, but seems not relevant:

=================================== FAILURES ===================================
_______________ AstroidManagerTest.test_module_is_not_namespace ________________

self = <tests.test_manager.AstroidManagerTest testMethod=test_module_is_not_namespace>

    def test_module_is_not_namespace(self) -> None:
        self.assertFalse(util.is_namespace("tests.testdata.python3.data.all"))
        self.assertFalse(util.is_namespace("__main__"))
>       self.assertFalse(
            util.is_namespace(list(EXT_LIB_DIRS)[0].rsplit("/", maxsplit=1)[-1]),
        )
E       AssertionError: True is not false

tests/test_manager.py:138: AssertionError
=========================== short test summary info ============================
FAILED tests/test_manager.py::AstroidManagerTest::test_module_is_not_namespace - AssertionError: True is not false
====== 1 failed, 1483 passed, 50 skipped, 14 xfailed, 1 xpassed in 12.88s ======

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #2117 (1f4d9a1) into main (c1f8ca8) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2117      +/-   ##
==========================================
+ Coverage   92.76%   92.80%   +0.04%     
==========================================
  Files          94       94              
  Lines       10972    10955      -17     
==========================================
- Hits        10178    10167      -11     
+ Misses        794      788       -6     
Flag Coverage Ξ”
linux 92.52% <100.00%> (+0.04%) ⬆️
pypy 88.08% <100.00%> (+0.03%) ⬆️
windows 92.35% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
astroid/arguments.py 100.00% <100.00%> (+4.10%) ⬆️

@Pierre-Sassoulas Pierre-Sassoulas added Needs review πŸ” Needs to be reviewed by one or multiple more persons Bug πŸͺ³ backport maintenance/2.15.x labels Apr 13, 2023
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.

Thanks ! I feel like there's a lot of place that could use safe_infer too in astroid.

DanielNoord
DanielNoord previously approved these changes Apr 13, 2023
@tusharsadhwani
Copy link
Contributor Author

A test failed due to the change it seems:

        node = extract_node(code)
        assert isinstance(node, nodes.NodeNG)
        result = node.inferred()
        assert len(result) == 2
        assert isinstance(result[0], nodes.Dict)
>       assert result[1] is util.Uninferable
E       assert <Dict.dict l.4 at 0x113aa9610> is Uninferable
E        +  where Uninferable = util.Uninferable

tests/test_inference.py:1458: AssertionError
____________________________________ AstroidManagerTest.test_module_is_not_namespace ____________________________________

@tusharsadhwani tusharsadhwani changed the title Use safe_infer to infer value for Starred node in _unpack_args Use safe_infer in _unpack_args and _unpack_keywords Apr 14, 2023
@jacobtylerwalls jacobtylerwalls removed the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Apr 14, 2023
@jacobtylerwalls
Copy link
Member

A test failed due to the change it seems:

The new result shows an improved inference result. The test was added just to test a crash, and it's being unnecessarily specific in comparing against Uninferable. I'll push an update.

@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Apr 15, 2023
@jacobtylerwalls jacobtylerwalls added this to the 2.15.2 milestone Apr 15, 2023
@jacobtylerwalls jacobtylerwalls modified the milestones: 2.15.2, 2.16.0 Apr 15, 2023
@jacobtylerwalls
Copy link
Member

This has the potential to raise more pylint messages as inference is changing, so I don't think it should be backported.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Could you add a changelog entry for 2.16?

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@jacobtylerwalls jacobtylerwalls enabled auto-merge (squash) April 15, 2023 14:00
@jacobtylerwalls jacobtylerwalls merged commit 495581f into pylint-dev:main Apr 15, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 3.0 Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug πŸͺ³ pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive no-value-for-parameter due to inference edge case
4 participants