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

Option to ignore in-line comments #3450

Closed
FSpanhel opened this issue Dec 18, 2022 · 8 comments
Closed

Option to ignore in-line comments #3450

FSpanhel opened this issue Dec 18, 2022 · 8 comments
Labels
T: enhancement New feature or request

Comments

@FSpanhel
Copy link

FSpanhel commented Dec 18, 2022

Consider this code snippet

import datetime as dt

default_date = dt.date.today() - dt.timedelta(1)  # my long comment appears here # fmt: skip

After running black in https://black.vercel.app/ this becomes

import datetime as dt

default_date = dt.date.today() - dt.timedelta(
    1
)  # my long comment appears here # fmt: skip

which I find really confusing and by far inferior to the original formatting. Note that black has formatted the code although the line ended with #fmt: skip (!) (probably related to #3009). Indeed, black does not format the code when I remove #fmt: skip because the line length is then small enough.

So I wonder whether it makes sense to make black ignore in-line comments?

At the moment the only possible solution to avoid this ugly formatting is

  1. Use

    import datetime as dt
    
    # fmt: off
    default_date = dt.date.today() - dt.timedelta(1)  # my long comment appears here
    # fmt: on

    which I don't like because I then have to add two empty lines for an in-line comment.

  2. Put the comment on an empty line

    import datetime as dt
    
    # my long comment appears here
    default_date = dt.date.today() - dt.timedelta(1)

    which I don't like because this also adds and additional line of code and is not useful if one uses chall chains and wants to add a comment to a particular line of the chain.

@torext
Copy link

torext commented Jul 10, 2023

I just wanted to voice my concern with this as well. Even setting the non-respecting of fmt: skip aside, I actually don't think Black should be formatting comments at all! Comments are not even parsed by Python and could be anything from just a very long comment to commented code; either way reformatting those is dangerous and, frankly, unnecessary. I only now realised Black was doing this and am quite shocked given the otherwise sensible behaviour it has...

Can we please please please have an option for Black to ignore comments completely?

EDIT: I am running with the --preview flag enabled; guessing that's where this is coming from? Thing is, I'd like Black to format long strings (e.g. messages to be printed in a warning etc.), but I would like it to ignore comments. Any way to achieve this currently as removing --preview also removes the long string formatting...

@FSpanhel
Copy link
Author

FSpanhel commented Jul 10, 2023

I assume that the developers are quite busy, but I would be very happy to receive some feedback from them on this topic after 7 months 🙂

At the moment, I try to use the second workaround in #3450 (comment) to deal with this problem.

However, this is quite annoying, because, as pointed out above, the simplicity of in-line comments is lost.

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Jul 10, 2023

There is a clear bug here in that # fmt: skip doesn't work if combined with another comment. That is being tracked in #3460 #3330.

Otherwise, I'm not sure what it means to "ignore" in-line comments as this issue asks for. Does it mean to ignore them when computing the line length, potentially leaving lines longer than the requested line length? I don't think that would be an improvement.

@torext
Copy link

torext commented Jul 11, 2023

Otherwise, I'm not sure what it means to "ignore" in-line comments as this issue asks for. Does it mean to ignore them when computing the line length, potentially leaving lines longer than the requested line length? I don't think that would be an improvement.

Yes, ignore them when computing line length essentially. In other words, pretend they are not even there. I'd have thought that's what Black does by default, especially given this seciton in the Black code style guide: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#comments.

@FSpanhel
Copy link
Author

FSpanhel commented Jul 11, 2023

Otherwise, I'm not sure what it means to "ignore" in-line comments as this issue asks for. Does it mean to ignore them when computing the line length, potentially leaving lines longer than the requested line length? I don't think that would be an improvement.

Yes, this is what I originally wanted. The reason is that our codebase has some lengthy in-line comments. Applying black to such code results in a formatting which we find confusing...

For instance,

import datetime as dt

default_date = {}
default_date['1'] = dt.date.today() - dt.timedelta(1)  # my long comment appears here # fmt: skip
default_date['2'] = dt.date.today() - dt.timedelta(2)  # my long comment appears here # fmt: skip
default_date['3'] = dt.date.today() - dt.timedelta(3)  # my long comment appears here # fmt: skip
default_date['4'] = dt.date.today() - dt.timedelta(4)  # my long comment appears here # fmt: skip
default_date['5'] = dt.date.today() - dt.timedelta(5)  # my long comment appears here # fmt: skip

becomes

import datetime as dt

default_date = {}
default_date["1"] = dt.date.today() - dt.timedelta(
    1
)  # my long comment appears here # fmt: skip
default_date["2"] = dt.date.today() - dt.timedelta(
    2
)  # my long comment appears here # fmt: skip
default_date["3"] = dt.date.today() - dt.timedelta(
    3
)  # my long comment appears here # fmt: skip
default_date["4"] = dt.date.today() - dt.timedelta(
    4
)  # my long comment appears here # fmt: skip
default_date["5"] = dt.date.today() - dt.timedelta(
    5
)  # my long comment appears here # fmt: skip

which we find quite hard to read.

Thus, making black ignore comments would be one solution to have a "nicer" formatting in these cases.

However, after reconsideration, I think that ignoring in-line comments may not the best option because PEP8 also considers in-line comments for the line length (our code base didn't so far).

Instead of ignoring in-line comments, maybe it would be better if black could put in-line comments above the code if they exceed the line length so that the above code example is formatted as

import datetime as dt

default_date = {}
# my long comment appears here # fmt: skip
default_date['1'] = dt.date.today() - dt.timedelta(1) 
# my long comment appears here # fmt: skip
default_date['2'] = dt.date.today() - dt.timedelta(2)  
 # my long comment appears here # fmt: skip
default_date['3'] = dt.date.today() - dt.timedelta(3) 
 # my long comment appears here # fmt: skip
default_date['4'] = dt.date.today() - dt.timedelta(4)
 # my long comment appears here # fmt: skip
default_date['5' = dt.date.today() - dt.timedelta(5) 

This would honor the line length and also improve the formatting IMO.

The only downside of this approach is that it is not clear how to deal with in-line comments that appear in a method chain, e.g.,

bla = (
    df.groupby(["col_1", "col_2", "col_3"])  # it is very important to consider col_3 here because otherwise the grouping doesn't work
    .apply(lambda x: x.method(arg1=False, arg2=True))  # for safety reasons, set arg2=True, although this may not be required (to be checked!)
    .sort_values(["col_3"])  #  if we don't sort in an ascending order we have a problem when we reset the index next
    .reset_index(drop=True)
)

In this case, it is not sensible to put all in-line comments above the method chain. So in this case, black's formatting should not be changed (?).

@FSpanhel
Copy link
Author

FSpanhel commented Jul 11, 2023

Otherwise, I'm not sure what it means to "ignore" in-line comments as this issue asks for. Does it mean to ignore them when computing the line length, potentially leaving lines longer than the requested line length? I don't think that would be an improvement.

Yes, ignore them when computing line length essentially. In other words, pretend they are not even there. I'd have thought that's what Black does by default, especially given this seciton in the Black code style guide: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#comments.

This section mentions that Comments may sometimes be moved because of formatting changes which probably relates to the movement of in-line comments (?).

So maybe, if no new feature to black is added, one could extend the documentation in this regard so that people better know what to expect?

@torext
Copy link

torext commented Jul 11, 2023

Otherwise, I'm not sure what it means to "ignore" in-line comments as this issue asks for. Does it mean to ignore them when computing the line length, potentially leaving lines longer than the requested line length? I don't think that would be an improvement.

Yes, ignore them when computing line length essentially. In other words, pretend they are not even there. I'd have thought that's what Black does by default, especially given this seciton in the Black code style guide: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#comments.

This section mentions that Comments may sometimes be moved because of formatting changes which probably relates to the movement of in-line comments (?).

So maybe, if no new feature to black is added, one could extend the documentation in this regard so that people better know what to expect?

"...may sometimes be moved because of formatting changes" sounds more like that some other formatting change that would've happened anyways without the comment being there might instead affect the comment. What we are observing here is that the comment itself is triggering the change. To me the docs don't seem to imply that at all and are extremely unclear in that regard...

@JelleZijlstra
Copy link
Collaborator

Thanks for the clarifications. In that case, I'm going to reject this issue.

Black in general aims to make all code fit within the line length limit. This is so that when code is read in an environment with fixed-size lines, the whole line is visible. Comments are part of the code and it's important that readers of the code can also read the comments, so they should also fit within the line length. Indeed, one of our most commonly liked rejected issues asks us to split up long comments into multiple lines (#1713). That suggests that many users would be upset if we allowed comments to break the line length in more cases.

I don't think the linked documentation is unclear, as it should be the default that any aspect of the code, including comments, is counted for line length purposes. However, if you would like to improve the docs, please file a PR and I'll take a look.

Similarly, if you have concrete proposals for improving how to handle comments (such as moving them to the line above), please open a new issue with explicit examples of how you would like code to be formatted. Make sure you don't overlap one of the existing issues in https://github.com/psf/black/issues?q=comment+is%3Aissue+label%3A%22F%3A+comments%22+, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants