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

Refactor: simplify get_scope_node by using a mapping dictionary #12919

Conversation

ToghrulMirzayev
Copy link

PR Description: Refactor get_scope_node to use a mapping dictionary

This PR refactors the get_scope_node function to improve its readability and maintainability. Previously, the function used a multi-level if-elif structure to match scope values with corresponding classes. I replaced this structure with a dictionary (scope_map) that maps each Scope to its corresponding class, reducing repetition and simplifying the code. Additionally, I also added a docstring with a clear explanation of the function's purpose.

Key improvements:

  • Reduction of repetition: The repeated calls to getparent() for different scopes were replaced with a single call using a mapped value from the scope_map dictionary.
  • Better readability: Using a dictionary makes it clearer which scope corresponds to which class, and makes it easier to add new scope mappings if necessary.
  • This should make the code easier to understand and extend while maintaining the same functionality.
  • A docstring explaining the purpose of the function

@The-Compiler
Copy link
Member

FWIW, I don't think the (very small) improvement in readability is worth the lost type safety in various regards:

  • Comparing strings instead of enum members (this one could be fixed though)
  • Need a type-ignore for more than what was originally intended by it
  • Losing the exhaustiveness-checking via assert_never()

Given that the current form was introduced deliberately in bf09e77 and this commit pretty much reverses that, I'm -1 on this.

@ToghrulMirzayev
Copy link
Author

FWIW, I don't think the (very small) improvement in readability is worth the lost type safety in various regards:

  • Comparing strings instead of enum members (this one could be fixed though)
  • Need a type-ignore for more than what was originally intended by it
  • Losing the exhaustiveness-checking via assert_never()

Given that the current form was introduced deliberately in bf09e77 and this commit pretty much reverses that, I'm -1 on this.

Hi. Thank you for the detailed feedback! To address your concerns, I’m considering modifying the code to reintroduce assert_never(scope) for exhaustiveness checking and to restrict # type: ignore[type-abstract] specifically to the Scope.Function case, as in the original version. This way, we can retain the benefits of exhaustiveness with the enum and keep the type-ignore usage minimal.

Would this approach address the type-safety and readability issues you highlighted?

Looking forward to hearing your thoughts!

@webknjaz
Copy link
Member

@ToghrulMirzayev could you tell us how you used ChatGPT here?

@ToghrulMirzayev ToghrulMirzayev closed this by deleting the head repository Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants