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

improve readability #9558

Closed
wants to merge 1 commit into from
Closed

improve readability #9558

wants to merge 1 commit into from

Conversation

fabinca
Copy link

@fabinca fabinca commented Apr 19, 2024

From the docs:

A tuple, as in isinstance(x, (A, B, ...)), may be given as the target to check against. This is equivalent to isinstance(x, A) or isinstance(x, B) or ... etc.

Type of Changes

Type

| ✓ | 🔨 Refactoring |

Description

Refs #XXXX

Closes #XXXX

From the docs:

    A tuple, as in isinstance(x, (A, B, ...)), may be given as the target to check against. This is equivalent to isinstance(x, A) or isinstance(x, B) or ... etc.
@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry labels Apr 19, 2024
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.

Thank you, it seems reasonable, but some tests are not passing anymore which is strange. Maybe there's some side effects here. Do oyu want to investigate ?

@Pierre-Sassoulas Pierre-Sassoulas added the Waiting on author Indicate that maintainers are waiting for a message of the author label Apr 22, 2024
@jacobtylerwalls
Copy link
Member

FunctionDef is a LocalsDictNodeNG, so from the failing tests I take it this needs to run twice to maintain the same behavior. I think the current way is fine (but might have benefitted from a comment in the code).

@fabinca
Copy link
Author

fabinca commented Apr 22, 2024

Ok, I see, thanks. Do you think it would make sense to look into refactoring the method

self._populate_type_annotations_function(
                       usage_node, all_used_type_annotations
                   )

to make it idempotent, or is this behaviour wanted?

@jacobtylerwalls
Copy link
Member

Thanks for the reply. I agree that the current design is not totally straightforward, but I just would prefer to see us direct energies toward other higher-impact areas than refactoring working code. I hope that makes sense 👍

@Pierre-Sassoulas Pierre-Sassoulas removed the Waiting on author Indicate that maintainers are waiting for a message of the author label Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants