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

Standardize type comments #4467

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

Conversation

Pedro-Muller29
Copy link

@Pedro-Muller29 Pedro-Muller29 commented Sep 27, 2024

Description

Issue #2097 - Type comments with extra (or zero) spaces after # and/or after : will be detected.
Pull Request #4152 - Type comments will also be formatted in a standardized way.

Since it's my first contribution, I am not sure if the creation of test_nodes.py to test the is_type_comment is correct, or if I should have tested it somewhere else.
I am also not sure whether this should be on preview or stable, or if I should change the documentation to point out this standardization.

Any feedback or suggestions for improvement are welcome.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary
  • Add or update tests if necessary
  • Add new or update outdated documentation

CHANGES.md Outdated
@@ -22,6 +22,7 @@
- Fix crashes involving comments in parenthesised return types or `X | Y` style unions.
(#4453)
- Fix skipping Jupyter cells with unknown `%%` magic (#4462)
- Standardize type comments to always have one space (#4467)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has to be in the preview style, not the stable style. You'll have to add a new member to the Preview enum and branch on that check; you can look for recent PRs that added preview features for examples.

This means that in the preview style we will normalize type comments on the next release, but in the stable style we will not, and when we release stable Black 25.0 in January this change will likely move to the stable style.

Copy link
Author

@Pedro-Muller29 Pedro-Muller29 Sep 28, 2024

Choose a reason for hiding this comment

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

I believe I’ve figured it out after following some recent PRs. However, I’m still unclear about your reference to adding a new member to the Preview branch. Are you referring to a Git branch? The PR I followed was merged directly into main, so I don’t think that’s the case. Could you clarify?

Copy link
Collaborator

Choose a reason for hiding this comment

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

By "branch" I mean an if statement, something like "if Preview.something in mode: do one thing; else: do another thing".

Copy link
Author

Choose a reason for hiding this comment

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

Got it! That clarifies how my changes only impact the preview mode.

Currently, neither of the functions I'm modifying includes mode in their parameters. Should I update the function signatures to incorporate it?

I also have a question regarding testing. I’ve made changes to the type_comment_syntax_error file to test the new standardized type comments with the --preview flag, removing the old tests in the process. Would it be better to create a new file and retain the old tests, or is there a preferred approach for handling this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you should add a mode parameter. And yes, it would be better to add a new file.

Copy link
Author

Choose a reason for hiding this comment

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

I've injected the mode as requested and added tests for the stable version. During this process, I had to make the Mode class hashable, as injecting mode into the call chain triggered multiple TypeError: unhashable type: 'Mode' errors in the unit tests.

However, I’m concerned that adding a hash function to the Mode class might introduce unintended side effects.

Would you consider the creation of this hash function appropriate, or is there a better way to address the TypeError: unhashable type: 'Mode' issue?

I am also facing some issues with the readthedocs build, it successfully runs on my local machine (MacOS), but on GitHub it seems to fail. The log says "Failed to checkout revision: aeca4e4", which is indeed the commit hash. Maybe some issue since I force pushed?

Thank you for your guidance!

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