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

Refactored function for purity and integrated with preview style #4543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iamaparnaojha
Copy link

Description:

This PR refactors the code to align with the feedback provided by @cobaltt7. Below are the key changes made:

Ensured Function Purity:

Refactored the function to ensure it remains pure by separating the modification logic into a different function.
The function now focuses solely on extracting the type annotation and avoids directly modifying any external state (e.g., leaf.value).
Integrated with Preview Style:

The modification logic has been moved to align with the preview style guidelines, ensuring better maintainability and clarity.
Code Formatting and Linting:

Used psf/black to format the code for consistent styling.
Resolved any linting issues detected by flake8.
Testing:

Verified changes locally with unit tests to ensure the refactor did not break any existing functionality.
Testing Steps:

Ran unit tests for the updated function to validate correctness.
Verified the integration with the preview style in the context of the overall system.
Relevant Issue or Context:
Addresses the feedback provided in the recent code review for improving the function's design and adhering to project guidelines.

Additional Notes:
Let me know if further refinements are needed. Looking forward to your feedback! 😊

@MeGaGiGaGon
Copy link
Contributor

I see you previously opened PR #4542, which Cobalt commented on and these changes are in reference to. Please only have one PR open for a change at a time. By keeping all related work on a single PR, it is much easier to track the history and discussion of a change, while minimizing merge conflicts.

Assuming you decide to keep this PR open and close the other one, here are my thoughts. If you instead decide to keep the other PR open and close this one, one of us can copy the contents of this message there.

Cobalt's comments on #4542 are

I think this function should probably stay pure, maybe put the modification part somewhere else

Also, this will need to go in the preview style

While it looks like you have split the modification part out, you are not using it anywhere, so normalize_type_comment is currently dead code. You should insert calls to it where nessacary, gated behind a check for a relevantly named preview flag.

It also looks like you have flake8 failures in the CI

src/black/nodes.py:938:96: B950 line too long (95 > 80 characters)
src/black/nodes.py:943:103: B950 line too long (102 > 80 characters)

Those should be fixed. If you haven't yet, read the contributing basics for a guide on all the commands to run on your PR/changes to ensure CI is happy.
You also will need a changelog entry, which is also covered in the above link.

@MeGaGiGaGon
Copy link
Contributor

Also, if you plan to keep using this issue please edit your original message to be closer to/a copy of your opening message in #4542

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.

2 participants