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
Reflows of inline comments can cause misalignment #379
Comments
Thanks for the report! My feeling is that your first example is "working as designed" and probably can't be reasonably fixed. The last comment lives outside the brackets, and it's reasonable for Black to keep it outside the brackets; how is Black supposed to know that it "goes with" the argument on that line rather than being a comment on the whole bracketed expression? It's not hard to move after reformatting, and then Black will keep it there. The second example might be fixable, I'm not sure without looking into it more closely. |
Thanks that's totally reasonable, I didn't consider that POV but I'm fine with it. The second example seems like a genuine bug (not an enhancement), though, whether or not it's fixable. |
Judgment call I guess; I call it an enhancement because all comment placement is kind of best-effort by heuristic, there will never be a solution that everyone finds optimal for all cases. Even in that case it's easy to see what Black is doing: it's keeping the comment on the same line as the same point in the AST. So the comment |
Ah, yeah that makes a lot of sense. In my head this was just an off-by-one error, but operating at the level of the ast obviously makes that a bunch more complicated. Thanks for the detailed response! |
I think I'm observing something related: Original:
Reformatted:
The |
I think it's impossible for black to distinguish between these two:
So it's unlikely that the formatted output will satisfy readers of both code snippets. So now we're talking about trading one group of people's confusion to another's unfortunately. I think putting the comment in the right place to begin with is the right call here (i.e. after @blaiseli your example is more similar to what was brought up here and as @ambv said there, is intentional. |
@zsol Indeed, it is more similar. I haven't understood the reason behind the intention, though. |
Because the black code style follows the "if it fits in one line, it should be one line" rule. This takes priority over comment placement. If it didn't fit, black would've split each argument into its own line, and then it has a chance to preserve the comment location (based on where it was in the original AST). |
@zsol I'm more concerned about example 2 which does not have anything to do with the closing delimiter. Your comment suggests that you closed this because of the other example, which I agree is impossible for black to disambiguate. # original
revert("!" +
one(2) + # extension
three(4) + # length
one(flapping) + # flapping info present
two(duration) + # duration
two(0)) becomes: revert(
"!"
+ one(2)
+ three(4) # extension
+ one(flapping) # length
+ two(duration) # flapping info present
+ two(0) # duration
) |
Ah you're right, I got confused because example 2 is the first one now :) I concur with @carljm for this one |
This is somewhat tangential, but I wanted to solicit whether this was expected behavior and if so whether I should start a new issue. Without a comment, formatted with black ( return (
df_day.assign(date=date, fund_ticker=ticker)
.pipe(lambda x: x.loc[x["Shares"].notnull()])
.assign(Shares=df_day["Shares"].astype("float64"))
) Adding a comment, before formatting return (
df_day.assign(date=date, fund_ticker=ticker)
.pipe(lambda x: x.loc[x["Shares"].notnull()])
# arrives as int sometimes
.assign(Shares=df_day["Shares"].astype("float64"))
) With a comment, after formatting with black: return (
df_day.assign(date=date, fund_ticker=ticker).pipe(
lambda x: x.loc[x["Shares"].notnull()]
)
# arrives as int sometimes
.assign(Shares=df_day["Shares"].astype("float64"))
) Without thinking about the more general problem, the second example here is ideal - the comment can be within a fluent expression and wouldn't change the fluent expression formatting. Thanks again for such an awesome library and vision! |
@max-sixty, this should be a separate issue. This is weird and should still be fluent. Looks like a bug to me on first read. |
I find this a inconvenient too:
becomes:
|
This issue, and #195 could both be fixed by having black pin the comment to the expression at the beginning of the line that the comment was on, rather than the end of the expression. That seems like it could present other pathological cases, but none are jumping to mind at the moment. |
Note to self: the problem to solve here is the example in #379 (comment). |
Note to self: type comments have special handling now to make them more sticky. @graingert suggests we use this logic for all comments. |
I am closing a number of issues as duplicates because they all share the common theme of Black moving comments to unfortunate places. Any solution to this issue should consider the cases mentioned in the duplicates and look for ways to improve them. |
Operating system: macOS
Python version: 3.6
Black version: 18.6b4
Does also happen on master: yes
edit: moved original first example to end because it's reasonable behavior
With this (example 2), black moves everything down one so it is aligned incorrectly:
becomes:
example 1 (reasonable behavior, original text preserved):
Black moves the last comment to a weird place, while handling most other lines correctly:
The text was updated successfully, but these errors were encountered: