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 #2698

Closed
wants to merge 4 commits into from
Closed

Standardize type comments #2698

wants to merge 4 commits into from

Conversation

13Ducks
Copy link

@13Ducks 13Ducks commented Dec 15, 2021

Description

Fixes #2097 and standardizes type comments to always have 1 space after the # and 1 space after the :

As per the issue, the functions is_type_comment() and contains_pragma_comment() have been modified to return true even with any extra whitespace. Type comments without non-breaking spaces are now stripped of any extra whitespace as well.

Waiting on confirmation from the issue that spaces before the : are acceptable (# type : something).

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

^ Is documentation needed for this change? If so, I can add it.

@13Ducks 13Ducks marked this pull request as draft December 15, 2021 14:28
@13Ducks 13Ducks marked this pull request as ready for review December 15, 2021 17:00
@ichard26
Copy link
Collaborator

ichard26 commented Dec 30, 2021

This was discussed on Python Discord but the summary of it is that we can't really normalize type comments until all of the major AST libraries that provide first-class type comment support are liberal in the whitespace they understand. It wouldn't be good to have a type comment that's ignored by typed-ast transformed into one that is noticed which then affects a type checker like mypy.

We might be able to normalize type comments on newer Python versions since typed-ast is mostly used to provide Python 2 support but this will need further investigatio.

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I don't remember the discussion @ichard26 alludes to above. Could you provide some details on how existing tools (typed-ast, pyright, pyre, ...) interpret spaces in type comments?

@github-actions
Copy link

diff-shades results comparing this PR (505634a) to main (b517dfb). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 2 projects & 13 files changed / 36 changes [+18/-18]   │
│                                                        │
│ ... out of 2 072 601 lines, 10 153 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@ichard26
Copy link
Collaborator

@@ -271,7 +276,7 @@ def contains_pragma_comment(comment_list: List[Leaf]) -> bool:
pylint).
"""
for comment in comment_list:
if comment.value.startswith(("# type:", "# noqa", "# pylint:")):
if re.match(r"#\s*(type\s*:|pylint\s*:|noqa\s*)", comment.value):
Copy link

Choose a reason for hiding this comment

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

Suggested change
if re.match(r"#\s*(type\s*:|pylint\s*:|noqa\s*)", comment.value):
if re.match(r"#\s*(type\s*:|pylint\s*:|noqa\s*|nosec\s*)", comment.value):

Would be nice if Bandit is supported as well.

@13Ducks 13Ducks closed this by deleting the head repository Sep 1, 2022
@CarliJoy
Copy link

CarliJoy commented Sep 2, 2022

@13Ducks giving up on this MR ?

Pedro-Muller29 added a commit to Pedro-Muller29/black that referenced this pull request Sep 27, 2024
Pedro-Muller29 added a commit to Pedro-Muller29/black that referenced this pull request Sep 27, 2024
Pedro-Muller29 added a commit to Pedro-Muller29/black that referenced this pull request Sep 28, 2024
Pedro-Muller29 added a commit to Pedro-Muller29/black that referenced this pull request Sep 28, 2024
Pedro-Muller29 added a commit to Pedro-Muller29/black that referenced this pull request Sep 30, 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.

is_type_comment() is wrong
4 participants