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(bug) build fails on windows with autocrlf=lf #10463

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Eulbobo
Copy link
Contributor

@Eulbobo Eulbobo commented Jul 31, 2024

Changing end of file and properties String manipulation to use Properties class

close #10398

Changing end of file and properties String manipulation to use Properties class

close jhipster#10398
Copy link
Collaborator

@DamnClin DamnClin left a comment

Choose a reason for hiding this comment

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

I have some doubts on that. Does this keep the other parts of the file intact (not changing order, not deleting blank lines etc)???

@Eulbobo
Copy link
Contributor Author

Eulbobo commented Aug 1, 2024

No it does not, and I didn't think it should since there was no test for this :/

Order should be preserved, but empty lines will definitly go away.
And a file header comment is automatically generated with generation date for the file (can't remove it, can only provide a default value on properties.store(outputStream, comment))

If those points are mandatory behaviours for the component, they should be tested accordingly beforehand

@DamnClin
Copy link
Collaborator

DamnClin commented Aug 1, 2024

No it does not, and I didn't think it should since there was no test for this :/

Order should be preserved, but empty lines will definitly go away. And a file header comment is automatically generated with generation date for the file (can't remove it, can only provide a default value on properties.store(outputStream, comment))

If those points are mandatory behaviours for the component, they should be tested accordingly beforehand

For me they are, we learned that later on but I think this is really important. @pascalgrimaud any advice on this?

@murdos
Copy link
Contributor

murdos commented Aug 1, 2024

IMO these points are indeed mandatory.
We try to preserve as much as possible the existing source file, and the header will generate some additional noise in system versioning changes.

@Eulbobo
Copy link
Contributor Author

Eulbobo commented Aug 1, 2024

First step : adding tests to check those behaviours in existing class

  • order in file preserved
  • comment preserved
  • white line preserved
  • new key/values appended at the end of the file
  • new values appended at the end of current existing values for key

Something like this :
image
(yes, it works on main branch even on my setup, be with a very ugly fix i'm too ashamed to show here)

This should go in another pull request/issue

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.

Build fails on Windows if you cloned with autocrlf=input, eol=lf
3 participants