Fix #808: newlines after curly lambda #859

Merged
merged 1 commit into from Mar 27, 2017

Conversation

Projects
None yet
2 participants
@pjrt
Collaborator

pjrt commented Mar 27, 2017

Fixes issue #808 by introducing a new setting: newlines.afterCurlyLambda

This option can have one of three settings: never (default), always and
preserve.

  • never(default) removes all lines. Before this commit, this is what was happening
  • always inserts one line if there is non and removes extra lines if
    there are more. This means there is always one (1) line
  • preserve is a combination of never and always. If the user does
    not have an extra line, there it will not add an extra line (like
    never). If there user has one or more lines, it will replaces it
    with one line (like always).
Fix #808: newlines after curly lambda
Fixes issue #808 by introducing a new setting:
newlines.afterCurlyLambda.

This option can have one of three settings: never (default), always and
preserve.

* `never` removes all lines. Before this commit, this is what was happening
* `always` inserts one line if there is non and removes extra lines if
    there are more. This means there is always one (1) line
* `preserve` is a combination of `never` and `always`. If the user does
    not have an extra line, there it will not add an extra line (like
    `never`). If there user has one or more lines, it will replaces it
    with one line (like `always`).
+ case object always extends NewlineCurlyLambda
+ case object never extends NewlineCurlyLambda
+
+ implicit val newlineCurlyLambdaReader: Reader[NewlineCurlyLambda] =

This comment has been minimized.

@pjrt

pjrt Mar 27, 2017

Collaborator

I noticed that for every other Reader we just define it in the companion object but don't make it implicit, but instead do so in ScalafmtConfig instead. This didn't work for me (due to a macro error I couldn't understand. But i think it wanted the implicit when creating Newline() in the config's default param).

Putting the typeclass instance on the companion object fixes this, and it is proper typeclass useage anyways (avoid orphans). We should do it for all the other ones too.

@pjrt

pjrt Mar 27, 2017

Collaborator

I noticed that for every other Reader we just define it in the companion object but don't make it implicit, but instead do so in ScalafmtConfig instead. This didn't work for me (due to a macro error I couldn't understand. But i think it wanted the implicit when creating Newline() in the config's default param).

Putting the typeclass instance on the companion object fixes this, and it is proper typeclass useage anyways (avoid orphans). We should do it for all the other ones too.

This comment has been minimized.

@olafurpg

olafurpg Mar 27, 2017

Member

Good point. I agree that absolute readers like this one should exist in the companion object. Only the @ConfigReader generated readers are not implicit and need to be inside ScalafmtConfig.

due to a macro error I couldn't understand.

Yeah, metaconfig/macros can be tricky. I badly wanted to avoid rolling my own config library/macros but I couldn't find anything that checked all boxes at the time, see #316 I think I might be able to use circe path readers, but I didn't know about them at the time.

@olafurpg

olafurpg Mar 27, 2017

Member

Good point. I agree that absolute readers like this one should exist in the companion object. Only the @ConfigReader generated readers are not implicit and need to be inside ScalafmtConfig.

due to a macro error I couldn't understand.

Yeah, metaconfig/macros can be tricky. I badly wanted to avoid rolling my own config library/macros but I couldn't find anything that checked all boxes at the time, see #316 I think I might be able to use circe path readers, but I didn't know about them at the time.

@pjrt

This comment has been minimized.

Show comment
Hide comment
@pjrt

pjrt Mar 27, 2017

Collaborator

Would be great if we could modify settings inside of a .stat file (currently, you can only modify them once at the top, right?). That's low priority though.

Collaborator

pjrt commented Mar 27, 2017

Would be great if we could modify settings inside of a .stat file (currently, you can only modify them once at the top, right?). That's low priority though.

+ * }
+ * }}}
+ *
+ * If `always`, it will always add one empty line (opposite of `never`).

This comment has been minimized.

@pjrt

pjrt Mar 27, 2017

Collaborator

Now that I think about it, I'm not sure always is the correct name for this. It isn't really "always" since the single-line split takes precedence. force might work, but it also isn't really "forced".

@pjrt

pjrt Mar 27, 2017

Collaborator

Now that I think about it, I'm not sure always is the correct name for this. It isn't really "always" since the single-line split takes precedence. force might work, but it also isn't really "forced".

This comment has been minimized.

@olafurpg

olafurpg Mar 27, 2017

Member

Naming is hard...

How about forceBlankLine? I'm OK whichever route we go.

@olafurpg

olafurpg Mar 27, 2017

Member

Naming is hard...

How about forceBlankLine? I'm OK whichever route we go.

This comment has been minimized.

@pjrt

pjrt Mar 27, 2017

Collaborator

That wouldn't be any different than just force.

Part of me wants to leave it as always and rely on the fact that, as a general rule in Scalafmt, single-line is always tried before anything else. With that fact in mind always (and force) make sense.

@pjrt

pjrt Mar 27, 2017

Collaborator

That wouldn't be any different than just force.

Part of me wants to leave it as always and rely on the fact that, as a general rule in Scalafmt, single-line is always tried before anything else. With that fact in mind always (and force) make sense.

@olafurpg

LGTM 👍

I personally would prefer to avoid having such elaborate configuration option around a somewhat (IMO) unimportant formatting detail. However, it's simple to accommodate, esp. with such a great test suite 😄

+ case object always extends NewlineCurlyLambda
+ case object never extends NewlineCurlyLambda
+
+ implicit val newlineCurlyLambdaReader: Reader[NewlineCurlyLambda] =

This comment has been minimized.

@olafurpg

olafurpg Mar 27, 2017

Member

Good point. I agree that absolute readers like this one should exist in the companion object. Only the @ConfigReader generated readers are not implicit and need to be inside ScalafmtConfig.

due to a macro error I couldn't understand.

Yeah, metaconfig/macros can be tricky. I badly wanted to avoid rolling my own config library/macros but I couldn't find anything that checked all boxes at the time, see #316 I think I might be able to use circe path readers, but I didn't know about them at the time.

@olafurpg

olafurpg Mar 27, 2017

Member

Good point. I agree that absolute readers like this one should exist in the companion object. Only the @ConfigReader generated readers are not implicit and need to be inside ScalafmtConfig.

due to a macro error I couldn't understand.

Yeah, metaconfig/macros can be tricky. I badly wanted to avoid rolling my own config library/macros but I couldn't find anything that checked all boxes at the time, see #316 I think I might be able to use circe path readers, but I didn't know about them at the time.

+ * }
+ * }}}
+ *
+ * If `always`, it will always add one empty line (opposite of `never`).

This comment has been minimized.

@olafurpg

olafurpg Mar 27, 2017

Member

Naming is hard...

How about forceBlankLine? I'm OK whichever route we go.

@olafurpg

olafurpg Mar 27, 2017

Member

Naming is hard...

How about forceBlankLine? I'm OK whichever route we go.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 27, 2017

Member

Seems like the CI had some network error, I restarted the job, pls let me know if it misbehaves again. Me and @jvican are working on making the CI production-ready for the Scala platform modules.

Member

olafurpg commented Mar 27, 2017

Seems like the CI had some network error, I restarted the job, pls let me know if it misbehaves again. Me and @jvican are working on making the CI production-ready for the Scala platform modules.

@pjrt pjrt merged commit b1a0f13 into scalameta:master Mar 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment