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

Also check the typealias naming style for TypeAlias variables defined in functions. #8537

Merged
merged 3 commits into from
Apr 7, 2023

Conversation

yilei
Copy link
Contributor

@yilei yilei commented Apr 3, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Closes #8536

@yilei
Copy link
Contributor Author

yilei commented Apr 3, 2023

Btw, tests/test_check_parallel.py seems to be failing when I run tests locally at HEAD:

platform darwin -- Python 3.11.1, pytest-7.2.2, pluggy-1.0.0

...

=========================================================================================================== FAILURES ============================================================================================================
________________________________________________________________________ TestCheckParallelFramework.test_linter_with_unpickleable_plugins_is_pickleable _________________________________________________________________________

self = <test_check_parallel.TestCheckParallelFramework object at 0x10a4eb710>

    def test_linter_with_unpickleable_plugins_is_pickleable(self) -> None:
        """The linter needs to be pickle-able in order to be passed between workers"""
        linter = PyLinter(reporter=Reporter())
        # We load an extension that we know is not pickle-safe
        linter.load_plugin_modules(["pylint.extensions.overlapping_exceptions"])
        try:
            dill.dumps(linter)
>           raise AssertionError(
                "Plugins loaded were pickle-safe! This test needs altering"
            )
E           AssertionError: Plugins loaded were pickle-safe! This test needs altering

tests/test_check_parallel.py:252: AssertionError

@Pierre-Sassoulas Pierre-Sassoulas added the False Negative πŸ¦‹ No message is emitted but something is wrong with the code label Apr 3, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0b1 milestone Apr 3, 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.

Neat fix ! @DanielNoord do you have an opinion about the pickle test failing ?

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #8537 (a5e55c8) into main (cb255ea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8537   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files         174      174           
  Lines       18366    18368    +2     
=======================================
+ Hits        17615    17617    +2     
  Misses        751      751           
Impacted Files Coverage Ξ”
pylint/checkers/base/name_checker/checker.py 98.59% <100.00%> (+<0.01%) ⬆️

@DanielNoord
Copy link
Collaborator

Can we reproduce? If not I'd say just merge this

@github-actions

This comment has been minimized.

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.

Reading the primer it seems some variable that are only typed are confused for a TypeAlias. For example in sentry: resolved_orderby: Union[str, SelectType, None]

@yilei
Copy link
Contributor Author

yilei commented Apr 4, 2023

@Pierre-Sassoulas Is "primer" the Primer actions run on this PR? Could you please point to me where the relevant outputs are? I checked those jobs but can only find generic logs without info like sentry / resolved_orderby: Union[str, SelectType, None]. Thanks!

@yilei
Copy link
Contributor Author

yilei commented Apr 4, 2023

@Pierre-Sassoulas Ah sorry never mind, just found out it's in a separate comment right after I posted my question. Taking a look now.

@yilei
Copy link
Contributor Author

yilei commented Apr 7, 2023

Rebased, the local variable declaration issue should have been fixed by #8541.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

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

This comment was generated for commit a5e55c8

Copy link
Collaborator

@DanielNoord DanielNoord 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 Do you want a review as well?

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.

Neat !

@Pierre-Sassoulas Pierre-Sassoulas merged commit b63c8a1 into pylint-dev:main Apr 7, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0a7 May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Negative πŸ¦‹ No message is emitted but something is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeAlias defined inside functions doesn't respect the typealias naming style.
3 participants