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

Feature/logical split #1126

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

jesse-sony
Copy link

First time submitting so there's probably a bunch of stuff I'm doing wrong and we'll have to work out the licensing stuff.

This is a PR to handle the issues in #844 where when there is a line that contains logical operators and we split on an operator, we want to split on all operators in that logical line.

I added a new knob to allow this feature to be toggled on. It is off by default for all styles to preserve the existing functioning of the styles.

I haven't pushed up any tests with this, so that will have to happen before this is merged obviously. However as part of my local testing I ran into some edge cases where you had to be able to set SPLIT_PENALTY_LOGICAL_OPERATOR and have it be respected. But the code as it previously existed would ignore penalties if the initial parse set any penalty value even if the desired penalty was higher than what the parse decided on. This prevented the line wrapping from being correct in some extreme edge cases. To get around that, I updated logical line to use the higher SPLIT_PENALTY_LOGICAL_OPERATOR but only in the case where the new knob was turned on. I was hesitant to change the normal (existing) behavior of that method, though I do believe that it probably should be considered at some point because higher penalties should probably propagate.

@google-cla
Copy link

google-cla bot commented Aug 25, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jesse-sony jesse-sony force-pushed the feature/logicalSplit branch from 41a3569 to 8e76277 Compare August 29, 2023 19:03
@jesse-sony jesse-sony force-pushed the feature/logicalSplit branch from 8e76277 to 4befa03 Compare October 18, 2023 18:46
@bwendling
Copy link
Member

Thanks for working on this! I like the direction you're going here. :-)

@jesse-sony
Copy link
Author

still working through all the internal hoops to get added to Sony's group that is approved to submit code to Google, but getting closer! Hopefully in next week or so we'll be able to progress on this for real

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

Successfully merging this pull request may close these issues.

None yet

2 participants