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

Distinction between Import braces and string interpolation braces #424 #454

Merged
merged 1 commit into from Oct 6, 2016

Conversation

Projects
None yet
3 participants
@hasumedic
Contributor

hasumedic commented Sep 20, 2016

This PR is a proposed fix for #424.

An important note would be that the proposed solution splits the "left brace" case into two to help distinguish between an import brace and a string interpolation one. The existing code contemplated both cases within the same block, creating multiple "inner" boolean checks to distinguish between the two cases. Keeping them separate allows for more fine tuning in the future and IMHO, simpler code to reason about due to less branching logic.

I'm not quite sure if this is the right way to solve it, please let me know (tests in particular!)

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 20, 2016

Current coverage is 84.77% (diff: 100%)

No coverage report found for master at cb524bf.

Powered by Codecov. Last update cb524bf...f76bf7b

codecov-io commented Sep 20, 2016

Current coverage is 84.77% (diff: 100%)

No coverage report found for master at cb524bf.

Powered by Codecov. Last update cb524bf...f76bf7b

@olafurpg

Looks great! I agree it is nicer to split those into separate branches. Just one minor comment and I'm happy to merge : )

PS. Sorry for the slow response, the email got lost in my inbox.

Show outdated Hide outdated core/src/main/scala/org/scalafmt/internal/Router.scala
case FormatToken(_, close @ RightBrace(), _)
if parents(rightOwner).exists(_.is[Import]) ||
rightOwner.is[Term.Interpolate] =>
val isImport = parents(rightOwner).exists(_.is[Import])

This comment has been minimized.

@olafurpg

olafurpg Sep 22, 2016

Member

Could you do change into isInterpolate = righOwner.is[...]? The parents method is really inefficient.

@olafurpg

olafurpg Sep 22, 2016

Member

Could you do change into isInterpolate = righOwner.is[...]? The parents method is really inefficient.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Oct 2, 2016

Member

Hey @hasumedic, any update? I would love to include this fix in 0.4.3, which I hope to release this coming week :)

Member

olafurpg commented Oct 2, 2016

Hey @hasumedic, any update? I would love to include this fix in 0.4.3, which I hope to release this coming week :)

@hasumedic

This comment has been minimized.

Show comment
Hide comment
@hasumedic

hasumedic Oct 3, 2016

Contributor

Sorry @olafurpg, had a bit rough couple of weeks. Will try to get this done for 0.4.3 ;)

Contributor

hasumedic commented Oct 3, 2016

Sorry @olafurpg, had a bit rough couple of weeks. Will try to get this done for 0.4.3 ;)

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Oct 3, 2016

Member

@hasumedic No stress, take your time.

Member

olafurpg commented Oct 3, 2016

@hasumedic No stress, take your time.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Oct 6, 2016

Member

LGMT 👍 Thanks a lot @hasumedic, I'm sure this fix will make many users happy!

Member

olafurpg commented Oct 6, 2016

LGMT 👍 Thanks a lot @hasumedic, I'm sure this fix will make many users happy!

@olafurpg olafurpg merged commit e6f1832 into scalameta:master Oct 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hasumedic hasumedic deleted the hasumedic:424-import-spaces-conflicting-interpolation branch Oct 7, 2016

@hasumedic

This comment has been minimized.

Show comment
Hide comment
@hasumedic

hasumedic Oct 7, 2016

Contributor

👍

Contributor

hasumedic commented Oct 7, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment