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

#356 Code with windows line endings after formatting still has windows endings #445

Merged
merged 2 commits into from Sep 18, 2016

Conversation

mmatloka
Copy link
Contributor

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thank you for this contribution. Only a few minor comments, otherwise we're good to merge 👍

}
}
} catch {
// TODO(olafur) add more fine grained errors.
case NonFatal(e) => FormatResult.Failure(e)
}
}

private[this] def containsWindowsLineEndings(code: String): Boolean =
code.contains("\r\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WindowsLineEnding?

import org.scalatest.FunSuite

class LineEndingsTest extends FunSuite with DiffAssertions {
val javadocStyle = ScalafmtStyle.default.copy(scalaDocs = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not default style?

@mmatloka
Copy link
Contributor Author

Corrected, squashed, pushed :)

@olafurpg
Copy link
Member

One last thought, should we have a flag to disable this new behavior?

@olafurpg
Copy link
Member

Whoops, I was fiddling with automatic publishing from travis (only possible from master branch) and just remembered that I disabled the tests suite to get faster experiments. Could you rebase on master and re-sync?

@sjrd
Copy link

sjrd commented Sep 17, 2016

One last thought, should we have a flag to disable this new behavior?

Yes please, so that scalafmtTest can assert that all my files have Linux line endings.

@codecov-io
Copy link

codecov-io commented Sep 17, 2016

Current coverage is 87.23% (diff: 92.00%)

Merging #445 into master will increase coverage by 0.07%

@@             master       #445   diff @@
==========================================
  Files            35         35          
  Lines          2228       2249    +21   
  Methods        2092       2116    +24   
  Messages          0          0          
  Branches        136        133     -3   
==========================================
+ Hits           1942       1962    +20   
- Misses          286        287     +1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 892298e...e478504

@olafurpg
Copy link
Member

@sjrd how about a flag like --lineEndings unix/windows/preserve where preserve will not change any line endings, unix enforces \n and windows enforces \r\n.

This option could be a sealed abstract class LineEnding and go into ScalafmtStyle. @mmatloka Here is an example on how to parse a custom type in the Cli: https://github.com/olafurpg/scalafmt/blob/892298eb38eec9590bef732478a7f2d8c0eadec8/cli/src/main/scala/org/scalafmt/cli/Cli.scala#L87

@mmatloka
Copy link
Contributor Author

@olafurpg as preserve you mean behaviour implemented in this PR or current master behaviour?

@olafurpg
Copy link
Member

@mmatloka Hmmm. Excellent question. I think preserve should be the behavior implemented in this PR. The current master branch behavior is buggy.

@olafurpg
Copy link
Member

@mmatloka For the readme, it should be @b{preserve} instead of @b(preserve). With the parentheses it assumes the argument is a variable name.

@@ -293,3 +293,14 @@
function(
argument1,
argument2)
@sect{--lineEndings}
Default: @b(preserve)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@b{preserve} will do the trick ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

@mmatloka
Copy link
Contributor Author

I have added the flag. Included in tests and docs, however I was not able to generate readme from scalatex locally, I don't know why yet (found : scalatex.site.Section.Proxy [error] required: scalatags.Text.all.Frag in not touched areas), so I have not checked how does "look like" the added configuration section. Please feel free to adjust the docs descriptions.

@olafurpg
Copy link
Member

LGTM. Thanks a lot for this contribution 😄 I expect to release very soon, the wifi is pretty bad at home so I may wait until tomorrow when I go to work.

@olafurpg olafurpg merged commit 60435c6 into scalameta:master Sep 18, 2016
@mmatloka
Copy link
Contributor Author

Thanks :)

@olafurpg
Copy link
Member

I am afraid the 0.3.2 release is taking longer than I expected. Is that OK? I have some exciting features in the pipeline that are almost complete and I want to bundle everything in a 0.4.

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

4 participants