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

Should # fmt: off be allowed as an inline comment? #790

Closed
joshkehn opened this issue Mar 29, 2019 · 9 comments
Closed

Should # fmt: off be allowed as an inline comment? #790

joshkehn opened this issue Mar 29, 2019 · 9 comments
Labels
C: configuration CLI and configuration S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: enhancement New feature or request

Comments

@joshkehn
Copy link

Sometimes I drop a import ipdb; ipdb.set_trace() line into a file when I'm debugging. black is super fast and cheerfully reformats this instantly on save to a multi-line:

import ipdb

ipdb.set_trace()

I ignored this a few times, and then figured I would write import ipdb; ipdb.set_trace() # fmt: off. I was surprised that black cheerfully (and again I want to stress this, super fast!) reformatted this into:

import ipdb

ipdb.set_trace()  # fmt: off

It kept the comment, spacing, everything. Such a good formatter!

But I wonder, could we allow an inline # fmt: off for this situation? Or for other one-liners or function definitions do avoid the need to wrap # fmt: off/# fmt: on blocks?

@zsol zsol added the T: user support OP looking for assistance or answers to a question label Mar 30, 2019
@zsol zsol changed the title [Question] Should #fmt: off be allowed as an inline comment? Should # fmt: off be allowed as an inline comment? Mar 30, 2019
@wbolster
Copy link
Contributor

wbolster commented Apr 9, 2019

as a work-around for this, i use this snippet in my editor nowadays:

__import__("pdb").set_trace()  # FIXME

which will be left as-is since it's a single statement.

@joshkehn
Copy link
Author

joshkehn commented Apr 18, 2019

@wbolster That's handy, thanks!

Separately the question still stands. I see two positions:

First, yes should do this because most linters allow inline comments. This is a feature I assumed should work based on historical experience with things like flake8, pylint, etc. It's also helpful/handy for small inline things, like maybe a large comprehension that I'm using to test with I can bang #fmt:off (or #fmt:ignore?) onto the end of the line and not have it shuffled on my auto-save lint.

Second, no, this isn't desirable because black is an opinionated formatter. It shouldn't make it easy to ignore (two lines to ignore a comment instead of an inline one). It's also likely this case is rare. I don't think clang-format allows inline ignores for a single line, and I haven't used yapf in three years. This is feature parity with auto-formatters which, as opposed to linters, operate on a larger scope (typically a block) than a specific line alone.

I've mostly convinced myself that the not doing this is easier and probably more consistent than doing this, unless someone has a use case or wants to further support the opinion for doing this? I'll put a patch together if the feeling is this should be explored further.

@dsedivec
Copy link

dsedivec commented May 3, 2019

See also #798.

@ambv
Copy link
Collaborator

ambv commented May 8, 2019

Practically, using # fmt: off as an inline comment is not something we can implement correctly with the current top down design because if your inline comment would be in the middle of an expression, the entire expression would need to be left alone. If your inline comment would be in the middle of a block which otherwise gets reindented, it now breaks code if you don't reindent it, too -- but it fails the contract if you do.

@wbolster
Copy link
Contributor

wbolster commented May 8, 2019

the intuitive (or least surprising) behaviour would be to leave the whole statement alone except for required reindentation (of each line of the statement).

@tebeka
Copy link

tebeka commented Jul 7, 2020

I'd like to give another use case. Sometiems I group long list of command line arguments in groups that makes sense, for example:

cmd = [
    'docker', 'run',
    '--detach',
    '--publish', '5432:5432',
    '--env', 'POSTGRES_PASSWORD=postgres',
    '--label', 'pg_testing',
    'postgres:12',
]

Which black turns into:

cmd = [
    'docker',
    'run',
    '--detach',
    '--publish',
    '5432:5432',
    '--env',
    'POSTGRES_PASSWORD=postgres',
    '--label',
    'pg_testing',
    'postgres:12',
]

I've love to be able to add a # fmt: off at the start of the list (in the cmd = [ line) and have black leave it as-is.

@StevenSong
Copy link

I'd like to give another use case. Sometiems I group long list of command line arguments in groups that makes sense:

@tebeka for large multiline blocks, I find it tolerable to wrap the section in # fmt: off/on like so

# fmt: off
cmd = [
    'docker', 'run',
    '--detach',
    '--publish', '5432:5432',
    '--env', 'POSTGRES_PASSWORD=postgres',
    '--label', 'pg_testing',
    'postgres:12',
]
# fmt: on

not sure if you knew about, but in general, I would still like inline # fmt: off

@wbolster
Copy link
Contributor

wbolster commented Aug 13, 2020

lo and behold:

cmd = [
    *["docker", "run"],
    *["--detach"],
    *["--publish", "5432:5432"],
    *["--env", "POSTGRES_PASSWORD=postgres"],
    *["--label", "pg_testing"],
    *["postgres:12"],
]

this creative use of ‘splat’ syntax (list unpacking) gives the same result, and black leaves this code as-is. yes, i realise that the aesthetics of this code are debatable, to say the least. 🙈

in fact, future me will likely deny that i suggested this at all. 😉

@ichard26 ichard26 added T: enhancement New feature or request and removed T: user support OP looking for assistance or answers to a question labels Apr 28, 2021
@JelleZijlstra
Copy link
Collaborator

We now support # fmt: skip.

@ichard26 ichard26 added C: configuration CLI and configuration S: accepted The changes in this design / enhancement issue have been accepted and can be implemented labels May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: configuration CLI and configuration S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants