-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Line too long: Comments #1713
Comments
I would love to solve this issue |
and for situations like
does this become:
or
|
I'd like to see this change. Maybe a reasonable first step would be to only format comments that are on a dedicated line, so splitting should cause no issues whatsoever? And in line with the upcoming string processing, should short comments also be combined into a single line? There probably are some complications with that though. # this
# is
# redundant
---
# this is redundant Just for reference, #2306 (which fixes #1017) will add a section to the documentation for comments that should be changed accordingly if this is implemented. |
This seems risky. Comments are often commented out code or other structured data. I think we should not merge short comments, but splitting long ones is probably OK. |
I would also really love to have this made possible, maybe as a default off option (although Black doesn't like having too many options, which I respect). For what it's worth, ClangFormat does reflow comments so they fit within the specified line length, and having used it for a few years now, it rarely causes any annoyance. |
I would go for a config option, something like Also if no-one is currently working on this, can you assign this issue to me, so I can work and open a PR. |
Most likely a new configuration option is a no-go. But seems like there is some acceptance for just splitting them, despite the edge cases. As I said, for starters we could do it for dedicated comment lines only, not inline comments. One discussion that should be had is the style of split. I quite like equalising the line lengths if one would be radically shorter. But that probably is also related to string processing, which I'm not familiar with (yet). # this is a very long comment that should probably be on multiple lines if I was to decide
# this is a very long comment that should probably be on multiple lines if
# I was to decide
# this is a very long comment that should
# probably be on multiple lines if I was to decide So I'd prefer the last option. |
Just to make this more difficult: I prefer the first. That will generally make diffs smaller, if say I decide to append some more text. |
That's a fair point as well! |
@felix-hilden Make ssense then to make this a default behavior, the inline scenario needs to be specified in the docs. I also prefer the 2nd one, but I believe that would require separate computation to calculate the lengths and then split the comments, I don't know how Either way, I like the problem in itself, to think algorithmically and come up with an optimal solution. |
Honestly I'd be tempted to say that it is bad code style to even have long inline comments. So leaving them out of formatting would be more convenient, but then again Black is specifically built to not only enforce but help in achieving a certain style, so we should also try to format them. But I might be thinking too narrowly 😅 |
As stated by @jvahue,
I don't know if either seem "right" to me. |
Just for reference, here are some examples of how ClangFormat handles inline comments: int main() {
int fairly_long_variable_name_1 = 9384759837459; // Short inline comment.
int fairly_long_variable_name_2 =
9384759837459; // Slightly longer inline comment.
int fairly_long_variable_name_3 =
9384759837459; // Really long inline comment, so long that it needs two
// lines.
} Maybe this behaviour is good? It allows inline comments to be infinitely long if you want to, but it's not super pretty (I don't think there is a pretty way to format this), so that might discourage very long inline comments. |
I'll say that I like the way that clang-format does it decently well and doing it their way would make what black does familiar to many users. If that output gets truly unwieldy, the user can just make it not be a block comment. int main ()
{
int long_variable_name_with_inline_comment =
9384759837459; // Lorem ipsum dolor sit amet, consectetur adipiscing elit,
// sed do eiusmod tempor incididunt ut labore et dolore
// magna aliqua. Ut enim ad minim veniam, quis nostrud
// exercitation ullamco laboris nisi ut aliquip ex ea
// commodo consequat. Duis aute irure dolor in reprehenderit
// in voluptate velit esse cillum dolore eu fugiat nulla
// pariatur. Excepteur sint occaecat cupidatat non proident,
// sunt in culpa qui officia deserunt mollit anim id est
// laborum.
// Lorem ipsum dolor sit amet, consectetur adipiscing elit,
// sed do eiusmod tempor incididunt ut labore et dolore
// magna aliqua. Ut enim ad minim veniam, quis nostrud
// exercitation ullamco laboris nisi ut aliquip ex ea
// commodo consequat. Duis aute irure dolor in reprehenderit
// in voluptate velit esse cillum dolore eu fugiat nulla
// pariatur. Excepteur sint occaecat cupidatat non proident,
// sunt in culpa qui officia deserunt mollit anim id est
// laborum.
int long_variable_name_with_block_comment = 9384759837459;
} |
I would say neither. Moreover, if you have to break in-line comments, the latter option is the most reasonable in terms of aesthetics. |
Dont use rev: stable suggestion from https://github.com/psf/black/blob/main/docs/integrations/source_version_control.md psf/black#420 Also black wont fix comments that are too long psf/black#1713 psf/black#182 Must manually fix these to stop flake8 from complaining
I want this feature so much, but I don't think it is possible to do it in a sensible way. Does this # This is me very long comment that is too long for one line. format to this? # This is me very long comment that is too long for one
# line. But what if I remove the # This is me long comment that is too long for one
# line. so that it fits now into one line? Does the comment stay as it is, even if it fits onto one line? Or does it change to # This is me long comment that is too long for one line. If the latter should hold, what happens with stuff like this: #! /bin/python
# test
#
# test Also, if you for example use # ---
# jupyter:
# jupytext:
# cell_metadata_filter: -all
# text_representation:
# extension: .py
# format_name: percent
# format_version: '1.3'
# jupytext_version: 1.11.5
# kernelspec:
# display_name: Python
# language: python
# name: test
# --- or markdown cells # %% [markdown]
# # Title
#
# # # Another title
#
# - the data comes from an autoregressive model with moderate autocorrelation
# - another bullet point
#
# And a formula:
# $$x \sim \mathcal{N}(0, 1)$$ In summary, I don't think formatting comments should be done by |
My opinion:
Yes.
It stays as it is, and you can manually rewrite it if you want it back on one line. ClangFormat does it this way, and in my experience it works well. |
The concern raised about commented code goes the other way too. If we have a line that is just under the limit, and is then commented, bringing it over the limit, making a new line could break it. Maybe it won't, but for example this will: if longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg:
pass
# Commented:
# if longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg:
# pass
# And reformatted for LL:
# if
# longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg:
# pass
# Uncommented and invalid syntax:
if
longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg:
pass |
That's a valid concern, and different from other languages (which would just parse the still valid code and reformat it properly if it gets wonky when un-commenting some code). I still think this is fine, though. When you uncomment some commented out code, it's not unreasonable to expect that you also fix it if its formatting gets weird. |
Wouldn't splitting inline comments upset linters? For example, with mypy: # This works...
import exchange_calendars as some_really_overly_long_name_just_to_offer_example # type: ignore[import]
# This doesn't...
import exchange_calendars as some_really_overly_long_name_just_to_offer_example # type:
# ignore[import] |
What's the status of this issue? In my project, we run I understand that there may be complications that I don't see, and it's fine to have it as a non-default option. I'll be happy to see it happen. |
@yaelbh are you saying that Black is causing the lines to become too long? As I understand it, this issue is about comments that are too long by themselves, not comments that get put on one line. I am pretty skeptical about should be splitting up comments. @ConstantinGahr's comment (#1713 (comment)) gives some good examples where it's problematic. |
I think the main issue is that Black fixes all of your too-long code lines, but not your too-long comment lines. So if you format to some line length with Black, and then enforce line length with another tool (which is reasonable), you'll end up with the comment lines still being too long, and your check will fail. It is possible for a tool like Black to re-wrap comment lines, as other code formatter tools already do. If so, it should be an opt-in feature, because it will mess up comments in codebases where comments don't adhere to a max line length. I absolutely think it would be useful. I also think we've discussed more or less all sides of this, and the next step should just be to decide whether Black should have this functionality or not. |
I'm becoming slightly in favor of having this as an opt-in. Either that, or not at all. We could start by just splitting lines that only have comments, to get rid of some complexity. That shouldn't be too hard. |
I'd like to state, that if we take care of some of the simple scenarios here, that it becomes next years default and it's only gated by the |
No, it's the other option that you state, of comments that are long in the first place. I expect a formatting tool like Black to split them for me. I understand that it can be problematic sometimes, but it's also a burden to have to split them by myself. |
Adding a `# type: ignore` comment to a couple of places triggered flake8's line too long check. Running `make reformat` did nothing for these - black has outstanding design issues with line length and comments. See psf/black#1713 for one example. Instead of changing the line structure to accommodate, ignore these two cases, at least until the types can be fixed and the comments removed. Signed-off-by: Mike Fiedler <[email protected]>
Adding a `# type: ignore` comment to a couple of places triggered flake8's line too long check. Running `make reformat` did nothing for these - black has outstanding design issues with line length and comments. See psf/black#1713 for one example. Instead of changing the line structure to accommodate, ignore these two cases, at least until the types can be fixed and the comments removed. Signed-off-by: Mike Fiedler <[email protected]>
* chore(deps): install mypy in lint.in Signed-off-by: Mike Fiedler <[email protected]> * chore(deps): include more types-* packages for mypy These were suggested from running mypy on the codebase. Signed-off-by: Mike Fiedler <[email protected]> * chore: configure mypy Set configuration for mypy. Exclude some of the subdirectories we are not interested in testing to speed up mypy execution. Ignore any 3rd party modules that we do not have types for yet. Added links that I could find to help track completion. Does **not** set `strict` mode yet, since that's a bigger lift. Signed-off-by: Mike Fiedler <[email protected]> * chore: add mypy runner script Eventually this command should fold into `bin/lint` and be removed. For now, it's a convenient execution wrapper. Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore dynamic properties Callables are receiving dynamic attributes, something that isn't "usual". See python/mypy#708 Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore lambdas See python/mypy#4226 Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore hybrid_property repeat definitions Part of the SQLAlchemy extensions, which do not yet have reliable stubs/plugins. Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore sqlalchemy declarative Should come along with sqlalchemy stubs. See: https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#using-declared-attr-and-declarative-mixins Signed-off-by: Mike Fiedler <[email protected]> * lint: a few more ignores Signed-off-by: Mike Fiedler <[email protected]> * lint: interface methods shouldn't use self Surfaced via mypy, corrected! Signed-off-by: Mike Fiedler <[email protected]> * lint: correct subclass path Surfaced via mypy, corrected. Unclear why this wouldn't have been caught by other tools. Signed-off-by: Mike Fiedler <[email protected]> * lint: rename internal variable mypy detected this as a type mismatch, as the internal variable name was shadowing the externally-supplied one, and changing the type. Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore flake8 line too long for ignored types Adding a `# type: ignore` comment to a couple of places triggered flake8's line too long check. Running `make reformat` did nothing for these - black has outstanding design issues with line length and comments. See psf/black#1713 for one example. Instead of changing the line structure to accommodate, ignore these two cases, at least until the types can be fixed and the comments removed. Signed-off-by: Mike Fiedler <[email protected]> * Revert "chore: add mypy runner script" This reverts commit fffeadb. * test: include mypy in lint execution Signed-off-by: Mike Fiedler <[email protected]> * chore(deps): include itsdangerous type stubs Until itsdangerous 2.0 is included, this types package is needed. Signed-off-by: Mike Fiedler <[email protected]>
i agree, black behaves very oddly with comments
becomes
which neither improves readability, nor keeps the comment on the same line as either the while loop or the condition, which it may have referred to with regards to comments, i would recommend:
IE do this
Or this:
|
Just checking in as its been a while... are there any plans to add support for shortening lines? |
For comment lines specifically, there is a good chance we'll never split them. There are other issues in
F: linetoolong
|
Maybe we just need to write a separate tool for this, outside of Black? While I personally would like to have this feature in Black, because it would be convenient for me, I acknowledge that it might not fit with Black's goals. |
* chore(deps): install mypy in lint.in Signed-off-by: Mike Fiedler <[email protected]> * chore(deps): include more types-* packages for mypy These were suggested from running mypy on the codebase. Signed-off-by: Mike Fiedler <[email protected]> * chore: configure mypy Set configuration for mypy. Exclude some of the subdirectories we are not interested in testing to speed up mypy execution. Ignore any 3rd party modules that we do not have types for yet. Added links that I could find to help track completion. Does **not** set `strict` mode yet, since that's a bigger lift. Signed-off-by: Mike Fiedler <[email protected]> * chore: add mypy runner script Eventually this command should fold into `bin/lint` and be removed. For now, it's a convenient execution wrapper. Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore dynamic properties Callables are receiving dynamic attributes, something that isn't "usual". See python/mypy#708 Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore lambdas See python/mypy#4226 Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore hybrid_property repeat definitions Part of the SQLAlchemy extensions, which do not yet have reliable stubs/plugins. Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore sqlalchemy declarative Should come along with sqlalchemy stubs. See: https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#using-declared-attr-and-declarative-mixins Signed-off-by: Mike Fiedler <[email protected]> * lint: a few more ignores Signed-off-by: Mike Fiedler <[email protected]> * lint: interface methods shouldn't use self Surfaced via mypy, corrected! Signed-off-by: Mike Fiedler <[email protected]> * lint: correct subclass path Surfaced via mypy, corrected. Unclear why this wouldn't have been caught by other tools. Signed-off-by: Mike Fiedler <[email protected]> * lint: rename internal variable mypy detected this as a type mismatch, as the internal variable name was shadowing the externally-supplied one, and changing the type. Signed-off-by: Mike Fiedler <[email protected]> * lint: ignore flake8 line too long for ignored types Adding a `# type: ignore` comment to a couple of places triggered flake8's line too long check. Running `make reformat` did nothing for these - black has outstanding design issues with line length and comments. See psf/black#1713 for one example. Instead of changing the line structure to accommodate, ignore these two cases, at least until the types can be fixed and the comments removed. Signed-off-by: Mike Fiedler <[email protected]> * Revert "chore: add mypy runner script" This reverts commit fffeadb. * test: include mypy in lint execution Signed-off-by: Mike Fiedler <[email protected]> * chore(deps): include itsdangerous type stubs Until itsdangerous 2.0 is included, this types package is needed. Signed-off-by: Mike Fiedler <[email protected]>
I'm going to go ahead and say that Black will not split comments.
|
Would it make sense in this case to turn this into some kind of stderr warning with rationale, so that people know that this was expected and not configurable? |
I think putting something in stderr will be noisy and annoying for users. I'd welcome improvements to the docs, though. |
Problem
Consider the below comment:
# this comment is longer than what is specified in line length arguments passed to black blah blah blah blah blah blah
If I run black on this file the comment remains intact, even though its clearly over the line length limit.
Split the comment to fit into the line length
Something like
# this comment is longer than what is
# specified in the line length
...
...
The same problem also exists for strings as well but I believe that's better left intact but for comments I think splitting them up into multiple lines makes sense.
If this is a feature that you guys believe will be a good addition to black, then I can work on this and open a PR.
Thanks
The text was updated successfully, but these errors were encountered: