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

Commented strings #2441

Closed
jonnyarnold opened this issue Aug 24, 2021 · 11 comments
Closed

Commented strings #2441

jonnyarnold opened this issue Aug 24, 2021 · 11 comments
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. T: style What do we want Blackened code to look like?

Comments

@jonnyarnold
Copy link

jonnyarnold commented Aug 24, 2021

Commented strings are currently realigned and repositioned.

Original:

# Matches:
#   li>*:first-child:before -> li
#   li:first-child>*:first-child:before -> li:first-child
#   *:before -> *
SELECTORS_TO_RETARGET = re.compile(
    r"(" +                         # Return this (as \1)...
    r"[^\:\>\s]+" +                # Anything before a pseudo (:), child (>) or descendant ( )
    r"(:first-child)?" +           # Include :first-child in the match if it's there
    r")" +                         # ...finish return match
    r"(\s?>\s?\*:first-child)?" +  # match on `> *:first-child` (which is usually output by Solid)
    r"::?before"                   # make sure we have a ::before pseudo-element (which we discard)
)

Formatted with Black 21.7b0:

# Matches:
#   li>*:first-child:before -> li
#   li:first-child>*:first-child:before -> li:first-child
#   *:before -> *
SELECTORS_TO_RETARGET = re.compile(
    r"("
    + r"[^\:\>\s]+"  # Return this (as \1)...
    + r"(:first-child)?"  # Anything before a pseudo (:), child (>) or descendant ( )
    + r")"  # Include :first-child in the match if it's there
    + r"(\s?>\s?\*:first-child)?"  # ...finish return match
    + r"::?before"  # match on `> *:first-child` (which is usually output by Solid)  # make sure we have a ::before pseudo-element (which we discard)
)

To enumerate the issues:

  • Comments that were aligned are now set flush against the code, decreasing readability.
  • Comments are moved onto the following line, misaligning the comment with the code.

Let me know if you need any further information! 👍

@jonnyarnold jonnyarnold added the T: style What do we want Blackened code to look like? label Aug 24, 2021
@MarcoGorelli
Copy link
Contributor

Hi @jonnyarnold - looks like this is out of scope, see #1904

@jonnyarnold
Copy link
Author

Thanks for the reply @MarcoGorelli.

I'm not too worried about the vertical comment alignment. However, I'd say that inline comments changing lines is a bug, rather than a style issue?

@wbadart
Copy link

wbadart commented Sep 22, 2021

I found this ticket after experiencing black moving around (thereby breaking) some type:ignore[...] comments for mypy. I also couldn't figure out the right combination of fmt:skip/type:ignore that would satisfy both tools and keep the comments to one line.

Is the official recommendation to just surround these lines in fmt:off/fmt:on? If so, sounds good. Otherwise, it would be very convenient to have another workaround, since the noise from several nearby fmt directives can add up quickly (especially since type:ignore comments tend to push line length up quite a bit).

@JelleZijlstra
Copy link
Collaborator

@wbadart we have code that tries to keep # type: ignore comments in the right place. If that's not working, please file a bug report (though make sure you're using the latest version).

@wbadart
Copy link

wbadart commented Sep 23, 2021

Ah, just ran a little experiment based on your comment. Looks like the code for pinning type ignores works for # type: ignore but not # type:ignore (both are recognized by mypy). Should I file a ticket for supporting the no-space version or is this the expected behavior?

@ichard26 ichard26 added the F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. label Sep 23, 2021
@ichard26
Copy link
Collaborator

Personally I think this should be supported. I think folding this into GH-2097 works well enough IMO, but I'll let Jelle weigh in.

@JelleZijlstra
Copy link
Collaborator

I'd vote for normalizing # type:ignore to # type: ignore actually. Formatting is our job, and type comments are a part of Python's grammar, so we get to choose how they're formatted.

@wbadart
Copy link

wbadart commented Sep 23, 2021

I think I agree that the format of comments like this do fall under black's purview. Would it be possible, in that case, to have black recognize # type:ignore and transform it to # type: ignore, rather than treat it like a regular comment (subject to regular rules about moving lines)?

@li1234yun
Copy link

Why assume that the current black configuration is optimal? It may be necessary to vote to evaluate the controversial areas for further optimization, rather than complacency.

Maybe most people think alignment is good! So We should not assume and need to evolve!

Black is very good, but it maybe not be perfect. It needs to evolve.

Thank you for providing so good tool!

@felix-hilden
Copy link
Collaborator

@li1234yun We don't assume it, hence a hefty number of issues like this labelled "design". But Black isn't built as a democracy of its users either, but rather according to the vision of its original author and current maintainers. Feel free to chime in though! And thank you for the kind words 👍

@JelleZijlstra
Copy link
Collaborator

The OP is another case of #379 (there are other issues about type ignore).

@JelleZijlstra JelleZijlstra closed this as not planned Won't fix, can't repro, duplicate, stale Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

7 participants