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

improve line separator handling #480

Merged
merged 1 commit into from Jan 6, 2022

Conversation

pblanchardie
Copy link
Contributor

@pblanchardie pblanchardie commented Jan 4, 2022

fix #477

  • .gitattributes
  • .editorconfig
  • .prettierrc
  • Add a lineSeparator project configuration and use it where applicable
  • Remove all uses of System.lineSeparator
  • Add utility method that replaces LF with custom lineSeparator

Some use of indentation constants were also committed as part of this PR

@pascalgrimaud supporting custom line separators would also required that we ensure applying them when modifying Java files (eg. updateExceptionTranslatorIT).

As LF is widely used in git repos by both Unix and Windows worlds, I'm not sure it's worth the effort after all. Let's re-think about forcing LF for everyone as good-practice. I don't know any advantage of using CRLF in source code, and it should never be used in Git.

Note that .gitattributes still overrides bat, cmd, ps1 and sh but it's not the case in editorconfig and prettierrc.

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #480 (04a38a6) into main (bfd88c2) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 04a38a6 differs from pull request most recent head f33f13d. Consider uploading reports for the commit f33f13d to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##                main      #480    +/-   ##
============================================
  Coverage     100.00%   100.00%            
+ Complexity       794       749    -45     
============================================
  Files            150       139    -11     
  Lines           2455      2317   -138     
  Branches          43        48     +5     
============================================
- Hits            2455      2317   -138     
Impacted Files Coverage Δ
...va/tech/jhipster/lite/common/domain/WordUtils.java 100.00% <ø> (ø)
...va/tech/jhipster/lite/common/domain/FileUtils.java 100.00% <100.00%> (ø)
...r/lite/generator/buildtool/maven/domain/Maven.java 100.00% <100.00%> (ø)
...tor/buildtool/maven/domain/MavenDomainService.java 100.00% <100.00%> (ø)
.../lite/generator/init/domain/InitDomainService.java 100.00% <100.00%> (ø)
...r/lite/generator/project/domain/DefaultConfig.java 100.00% <100.00%> (ø)
...hipster/lite/generator/project/domain/Project.java 100.00% <100.00%> (ø)
...ation/liquibase/domain/LiquibaseDomainService.java 100.00% <100.00%> (ø)
...logging/domain/SpringBootLoggingDomainService.java 100.00% <100.00%> (ø)
...ties/domain/SpringBootPropertiesDomainService.java 100.00% <100.00%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfd88c2...f33f13d. Read the comment docs.

@pblanchardie
Copy link
Contributor Author

It's now cleaner with project.getEndOfLine()

@pascalgrimaud Should we add endOfLine in ProjectDTO so the user can force it or leave it null for "automatic" mode?
Or only rely on some arbitrary file in the project to find eol so we don't ask dumb questions and find the answer ourselves?^^

For the moment, it's forced to LF with a TODO.

@pblanchardie pblanchardie force-pushed the end-of-line branch 2 times, most recently from 7772f17 to dbd6426 Compare January 4, 2022 14:31
@pblanchardie pblanchardie mentioned this pull request Jan 4, 2022
@pblanchardie
Copy link
Contributor Author

pblanchardie commented Jan 4, 2022

git patch fails with CRLF, maybe we have to force .patch files to be lf, or definitely force LF for everyone as we'll run into multiple issues with CRLF without bringing value.
I did not investigate further as git patches are very sensible to line endings.

we can also use the detection to check if we have LF, otherwise inform the user to enforce LF (at least during v0.x) + README.
up to you.

@pascalgrimaud
Copy link
Member

pascalgrimaud commented Jan 4, 2022

I didn't expect you provide a 1st solution so fast!

As LF is widely used in git repos by both Unix and Windows worlds, I'm not sure it's worth the effort after all. Let's re-think about forcing LF for everyone as good-practice. I don't know any advantage of using CRLF in source code, and it should never be used in Git.

I agree with this, maybe we should simply force LF for project -> in this case, we should change gitattribute as suggested and all the work here is lost ? (really sad)

@pblanchardie
Copy link
Contributor Author

Don't worry, the most important is not lost ;)

The code is now cleaner and platform independant, all tests pass and we can reuse detection to ensure LF: it will be better than failing on the git patch, as the error it produces is very hard to understand and can be very frustrating for the user.

We really needed to investigate on this windows related stuff anyway.

Changing gitattributes is not sufficient, as core.autocrlf is responsible for converting the working copy to crlf and is a global config. We could also check it to be false when runni g on local machine...

@pascalgrimaud
Copy link
Member

@pblanchardie : similar issue here jhipster/generator-jhipster#17115

@pascalgrimaud
Copy link
Member

@pblanchardie : can I merge this?

@pblanchardie
Copy link
Contributor Author

You can, but it may need further improvements.

@pascalgrimaud
Copy link
Member

Ok, so I'll wait and merge when the PR will be ready :-)

@pblanchardie
Copy link
Contributor Author

fixing conflicts!

@pblanchardie pblanchardie force-pushed the end-of-line branch 3 times, most recently from d26faf8 to b0d8a68 Compare January 5, 2022 21:25
@pblanchardie
Copy link
Contributor Author

git patch now works everywhere when *.patch files are forced to stay with lf

@pblanchardie
Copy link
Contributor Author

@pblanchardie : similar issue here jhipster/generator-jhipster#17115

I did read and was about to answer as I have some advice about what not to do, but I'll get back to it tomorrow ;)

this PR can be merged as-is, more to come in other PR.

@pblanchardie pblanchardie marked this pull request as ready for review January 5, 2022 21:42
@pascalgrimaud
Copy link
Member

Ok @pblanchardie : I'll merge it tomorrow, as I think I need to use your work in existing code:

  • I merge some PR, 1 hour ago and I used hard code \n I think

@pblanchardie
Copy link
Contributor Author

Ok @pascalgrimaud

Detection should be improved to ignore sh, vsproj, bat... that are not relevant because they are forced to lf/crlf, and binary files. Not big deal, planned.
Good night!

@pascalgrimaud pascalgrimaud merged commit 2d96ca0 into jhipster:main Jan 6, 2022
@pascalgrimaud
Copy link
Member

Huge work, thanks a lot @pblanchardie

@pblanchardie pblanchardie deleted the end-of-line branch January 6, 2022 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.lineSeparator is not relevant in all cases
2 participants