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

feat: Introduce hawkeye-maven-plugin #84

Merged
merged 13 commits into from
Aug 30, 2023
Merged

feat: Introduce hawkeye-maven-plugin #84

merged 13 commits into from
Aug 30, 2023

Conversation

spencercjh
Copy link

@spencercjh spencercjh commented Aug 25, 2023

RESOLVE #21

@spencercjh spencercjh marked this pull request as draft August 25, 2023 16:54
@spencercjh spencercjh changed the title Introduce hawkeye-maven-plugin feat: Introduce hawkeye-maven-plugin Aug 25, 2023
@tisonkun
Copy link
Member

tisonkun commented Aug 27, 2023

Thanks for your participantion @spencercjh!

This seems in a good direction. You may depend on the hawkeye-core module and following hawkeye-command and license-maven-plugin to wrap Mojo facades of LicenseChecker, LicenseFormatter and LicenseRemover, separately.

@spencercjh
Copy link
Author

spencercjh commented Aug 28, 2023

@tisonkun I have run ./mvnw spotless:apply but I found that the package import order is still not as same as yours.

@spencercjh spencercjh marked this pull request as ready for review August 28, 2023 03:37
@tisonkun
Copy link
Member

@spencercjh You're right that I don't add an ImportOrder rule here. Let me fix it today.

@tisonkun
Copy link
Member

@spencercjh You can merge #89 and rerun the command above.

@spencercjh
Copy link
Author

@spencercjh You can merge #89 and rerun the command above.

done

@tisonkun
Copy link
Member

Cool! You may try to add some tests or at least share some evidences tested locally so that we're sure it's a good starting point for Maven plugin support.

@spencercjh
Copy link
Author

Cool! You may try to add some tests or at least share some evidences tested locally so that we're sure it's a good starting point for Maven plugin support.

done

@tisonkun tisonkun self-requested a review August 29, 2023 06:59
@spencercjh
Copy link
Author

I will fix wrong tests ASAP.

@tisonkun
Copy link
Member

@spencercjh We're not in a hurry so just take your time :D

You can verify the tests locally first.

licenserc.toml Outdated Show resolved Hide resolved
@spencercjh
Copy link
Author

spencercjh commented Aug 29, 2023

@tisonkun

I found something unexpected on Windows which leads the failure of my tests.

image

Associated codes are here: https://github.com/korandoru/hawkeye/blob/main/hawkeye-core/src/main/java/io/korandoru/hawkeye/core/header/Header.java#L110-L116

All \r\n in firstLines were replaced by \n as fileHeader.

But there are still \r\n in expectedHeader.

I think it's a bug because of missing integration tests on Windows :D. Maybe we need another bug issue?

I tried to fix it on this commit and make tests pass on my Windows PC. If you mind it I can raise another PR to fix it.

@tisonkun
Copy link
Member

tisonkun commented Aug 29, 2023

Let me think of it..

The expected header should be generated from the content or file you provided instead of the tested files. So I may suggest that you force use \n instead of \r\n in the template (expected header), rather than do some in place ad-hoc fixup.

@spencercjh
Copy link
Author

Let me think of it..

The expected header should be generated from the content or file you provided instead of the tested files. So I may suggest that you force use \n instead of \r\n in the template (expected header), rather than do some in place ad-hoc fixup.

Do you mean to force the line separator of file hawkeye-core/src/main/resources/Apache-2.0.txt? Or force the line separator of the header file configured in the config toml file?

@spencercjh
Copy link
Author

spencercjh commented Aug 29, 2023

Now I found that on Windows if I don't update Header.java when I add the plugin to hawkeye-command and run ./mvnw hawkeye:check, it will fail and all of the files will be considered as missing header.

@tisonkun
Copy link
Member

Do you mean to force the line separator

After a closer look I noticed that we use endOfLine variable to build the expected header and update. Then I think we can do the same fixup as your previous commit does.

@tisonkun
Copy link
Member

Now I found that if I don't update

May you share the console output or screenshot? I'm not sure what's the issue here.

@spencercjh
Copy link
Author

image

It runs on b7cb7fd, the previous commit.

image

May you share the console output or screenshot? I'm not sure what's the issue here.

I just want to tell you the bug. I think there is no other issues besides of the endOfLine.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

It's a bit late now so I may not be quite clear.

Pending to merge tomorrow morning.

@tisonkun
Copy link
Member

Merging...

I'll prepare a release for delivering this plugin. Maybe you can try to write a few docs for the Maven plugin usage. Either in Usage section under README.md or in the hawkeye-maven-plugin folder should be fine.

@tisonkun tisonkun merged commit fbcdd4e into korandoru:main Aug 30, 2023
@spencercjh spencercjh deleted the feat/maven-plugin branch August 30, 2023 05:59
@tisonkun
Copy link
Member

Released as 3.3.0. We may add a skip option to these Mojos so that users can opt-out the plugin execution.

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.

Distribute as a Maven plugin
2 participants