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

Force else to break before anything inside it (fixes #616) #1032

Merged
merged 1 commit into from Mar 17, 2017
Merged

Conversation

jlongster
Copy link
Member

This is a more appropriate fix than #680. The problem is described more in #680 (comment). The only solution I can think of is to be very careful when objects are expanded. They can be naturally wrapped in the first pass, but the second one will see hard breaks and break everything above it. For the most part, that behavior is identical, but there are a few cases like this one where it wasn't breaking the code around it first.

This changes that and creates a new group that will break before the object breaks. That will force it to add the newline and stabilizes the output.

@vjeux
Copy link
Contributor

vjeux commented Mar 17, 2017

<3

Thanks for looking into it!

@jlongster
Copy link
Member Author

No problem!

Just realized the if part is now unstable, whoops. Need to play with this a little bit more.

@jlongster
Copy link
Member Author

So this is the only way to fix it. We need to create separate groups for the if and else parts so that they can break separately. It's the only way I can think to make the object expansion match in both places.

The core problem is the conditionalGroup which suppressed the hardline. It's a pattern we need to avoid, unfortunately. Suppressing that hard line means the measurer won't break the if/else expression but will break things inside of it.

I think this should be OK in real-world code. Hopefully it doesn't change too much code but I think it's pretty rare to mix bracket and no-bracket styles.

@jlongster jlongster requested a review from vjeux March 17, 2017 17:51
@vjeux
Copy link
Contributor

vjeux commented Mar 17, 2017

One less conditional group <3

I do think that it's rare enough to have both use a different bracket/no-bracket that we shouldn't support this use case.

@vjeux vjeux merged commit eeae443 into master Mar 17, 2017
@lydell lydell deleted the break-else branch September 12, 2017 06:44
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants