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

Handle lineSeparator conversion with tokens LF / CRLF #410

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Wykiki
Copy link

@Wykiki Wykiki commented Aug 23, 2021

This PR is linked to my comment on the issue #106

We are now able to provide the lineSeparator information with :

<license.lineSeparator>LF</license.lineSeparator>
<license.lineSeparator>CRLF</license.lineSeparator>

I tried to do the smallest change possible to avoid breaking anything, I'm not sure that it fits the codebase as I've not spent much time looking into the project.

@Wykiki Wykiki marked this pull request as ready for review August 23, 2021 15:17
@slawekjaranowski
Copy link
Member

Should be done in AbstractFileHeaderMojo we can do setter method for config property lineSeparator, we should also update documentation on this property.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

Please move to AbstractFileHeaderMojo

@Wykiki
Copy link
Author

Wykiki commented Jul 16, 2023

Updated to correct file, but not sure if setter is correctly picked up.
I wanna write test for that, should I duplicate the existing one like what's under src/it/update-file-header-test-mojo for this purpose ?

@slawekjaranowski
Copy link
Member

Please also update documentation for field in Java docs

    @Parameter(property = "license.lineSeparator")
    private String lineSeparator;

It will be used for generating description like in: https://www.mojohaus.org/license-maven-plugin/update-file-header-mojo.html#lineSeparator

@slawekjaranowski
Copy link
Member

You can duplicate src/it/update-file-header-test-mojo but can be simplified only for checking lineSeparator

@Wykiki
Copy link
Author

Wykiki commented Aug 7, 2023

Tried to implement tests, but it looks like line ending got replaced by system one when either \r\n or \n characters ends up as the lineSeparator property.

I may have more time to dig that later, but if you have any clue where this could happen, feel free to give advice !

I suspect something around following lines :

*/
package org.codehaus.mojo.license;

public class Lf
Copy link
Member

Choose a reason for hiding this comment

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

Even it is test file should follow java conventions, class should the same name as file.
Please also check other test files.

@slawekjaranowski
Copy link
Member

Please always build locally ... last change has issue reported by spotless.

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

3 participants