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

Fix 143 #259

Merged
merged 6 commits into from
Jun 29, 2018
Merged

Fix 143 #259

merged 6 commits into from
Jun 29, 2018

Conversation

s-trooper
Copy link
Contributor

This closes #143, this PR preserve original end of lines, disables auto insert/remove of EOLs by Formatter.
As it give control to line breaking to developer, it implicit solves #43, #130, #132, #133? but that of course depends of expectations of issue creators.

@s-trooper s-trooper changed the title fix-143 Fix 143 May 31, 2018
@stroborobo
Copy link

Hi @s-trooper!

Thank you very much for this change. Unfortunately it also adds semicolons at line endings where they are not needed, like multi line records etc., even with SemicolonAtEndOfLine disabled.

@s-trooper
Copy link
Contributor Author

Hi @stroborobo, thank you for the report. I have fixed the problem.

@nojaf
Copy link
Contributor

nojaf commented Jun 27, 2018

Hi @s-trooper , we looked at your PR. Looks good! Could you elaborate on the ignored unit tests?
@dungpa could you perhaps review this one please?

@nojaf nojaf requested a review from dungpa June 27, 2018 18:50
@s-trooper
Copy link
Contributor Author

s-trooper commented Jun 28, 2018

Hi @nojaf, i tested my RP by format the fantomas source, the ignored tests are some problems I had, that prevent the source code from being compiled after formatting. Since those issues are not caused by my RP (some stuff is related to F# Compiler Services), I used the ignored tests to document them for later.

Probably I had to put these tests in a separate branch, should I do that? Or just ignore them when merging the RP.

let z = [7; 8]
""" { config with SemicolonAtEndOfLine = false }
|> should equal """
let x = [ 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: why is this adding a ; when SemicolonAtEndOfLine = false is set? I guess it is unrelated to this PR

@s-trooper
Copy link
Contributor Author

Hi @dsyme, it is so the "SemicolonAtEndOfLine" does not have a reliable (i don't see any?) effect on lists even in the master branch. Whereby I think that my code has a similar problem too, which was only covered by the master issue. I will think how this can be solved but my priority is that formatted code compiles.

@nojaf
Copy link
Contributor

nojaf commented Jun 28, 2018

Hi @s-trooper , regarding the ignored tests. I'd prefer if they are left out of this PR. We could create separate issues if necessary.

@s-trooper
Copy link
Contributor Author

@nojaf, i've removed ignored tests

@nojaf nojaf merged commit 479b8ed into fsprojects:master Jun 29, 2018
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.

Option to preserve blank lines
4 participants