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

Add new line after logical operator #844

Open
Conchylicultor opened this issue Jun 23, 2020 · 6 comments
Open

Add new line after logical operator #844

Conchylicultor opened this issue Jun 23, 2020 · 6 comments

Comments

@Conchylicultor
Copy link
Member

Conchylicultor commented Jun 23, 2020

Currently, I don't think there is a way to have each condition element to have their own line.
Additionally, the current formatting do not respect the split_before_logical_operator.
All condition get clustered like:

if (
    very_long_variable_name is not None and verrry_long_variable_name.field > 0
    or very_long_variable_name.is_debugfs
):
  pass

I would like to format my code as:

if (
    very_long_variable_name is not None
    and verrry_long_variable_name.field > 0
    or very_long_variable_name.is_debugfs
):
  pass

My .config/yapf/style:

[style]
based_on_style = google
indent_width = 2
coalesce_brackets = True
dedent_closing_brackets = True
split_before_dot = True
split_before_logical_operator = True
swapnalshahil added a commit to swapnalshahil/yapf that referenced this issue Aug 17, 2020
@keli
Copy link

keli commented Dec 10, 2020

True. Adding a knob for that would be nice.

@MHannila
Copy link

I'd also like this. Currently this is the biggest problem with us using yapf on our codebase, because there are so many long if checks that'd become very hard to read.

@jesse-sony
Copy link

This is an issue for us as well. Looking at the code, though it might be somewhat interesting to implement. It would need a new knob for sure. Then I guess whenever a Logical operator token is found in MustSplit, you need to check the total line length (which I think means iterating over next_token until you find None. Does not appear to be an existing method for this or a way to access the LogicalLine from the token to get .last). If the total line length is over the desired column count, then return True for the operator.

Now that only works for splitting AFTER the operator, however. So I guess if the style has SPLIT_BEFORE_LOGICAL_OPERATOR set, then we have to always do a one token seek ahead and check for a Logical operator and perform the check at that time to force the split before we hit the logical operator.

This will have the impact, I think, of prioritizing logical operator wrapping over other types of wrapping, which I think is desirable but I'm not fully sure. Would need to see how it behaves in real code to get a good feel for that.

@jesse-sony
Copy link

That also doesn't take into account a long chain of logical operators, some of which are nested inside parens. I guess the calculation for the wrap of a logical operator should take into account the total length of the clause, if the operator is nested inside parens or the total length of the line if it isn't nested

jesse-sony added a commit to jesse-sony/yapf that referenced this issue Aug 25, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
jesse-sony added a commit to jesse-sony/yapf that referenced this issue Aug 29, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
@Conchylicultor
Copy link
Member Author

Yapf is mostly unmaintained. I would suggest you to switch to Black or Pyink (google version that supports 2-space indentation, single quote,...)

@jesse-sony
Copy link

There are still PRs being accepted and I'm happy to do the work to add the features we need in because black/pyink don't support the formatting we want. Worst case, I maintain our own internal fork of yapf.

jesse-sony added a commit to jesse-sony/yapf that referenced this issue Aug 30, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
jesse-sony added a commit to jesse-sony/yapf that referenced this issue Aug 30, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
jesse-sony added a commit to jesse-sony/yapf that referenced this issue Oct 18, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
jesse-sony added a commit to jesse-sony/yapf that referenced this issue Oct 18, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
jesse-sony added a commit to jesse-sony/yapf that referenced this issue Nov 17, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
jesse-sony added a commit to jesse-sony/yapf that referenced this issue Nov 17, 2023
This comes from the issue
google#844
where logical operators are bin-packed if the line is too long instead
of wrapping nicely one to each line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants