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

Maven: Add configuration plugin/pluginManagement #361

Merged
merged 4 commits into from
Dec 21, 2021

Conversation

swarajsaaj
Copy link
Contributor

Pre-requisite for #284

@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #361 (3438293) into main (524743c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##                main      #361   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       538       550   +12     
===========================================
  Files             93        93           
  Lines           1733      1777   +44     
  Branches          36        37    +1     
===========================================
+ Hits            1733      1777   +44     
Impacted Files Coverage Δ
...va/tech/jhipster/lite/common/domain/FileUtils.java 100.00% <ø> (ø)
...ildtool/generic/domain/BuildToolDomainService.java 100.00% <100.00%> (ø)
...ite/generator/buildtool/generic/domain/Plugin.java 100.00% <100.00%> (ø)
...ool/maven/application/MavenApplicationService.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%> (ø)
...pringboot/mvc/security/jwt/domain/JwtSecurity.java 100.00% <0.00%> (ø)
.../security/jwt/domain/JwtSecurityDomainService.java 100.00% <0.00%> (ø)
...y/jwt/infrastructure/rest/JwtSecurityResource.java 100.00% <0.00%> (ø)
...jwt/application/JwtSecurityApplicationService.java 100.00% <0.00%> (ø)

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 524743c...3438293. Read the comment docs.

@Test
void shouldGetPluginWithAdditionalElements() {
// @formatter:off
String expected = "<plugin>" + System.lineSeparator() +
Copy link
Member

Choose a reason for hiding this comment

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

maybe use text block ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Still we need to add a replaceAll("\n", System.lineSeparator()) to keep it OS friendly as Line endings in text blocks are always \n

Plugin plugin = fullPluginBuilder()
.additionalElements(
"""
<executions>
Copy link
Member

Choose a reason for hiding this comment

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

maybe try to respect indent with 2 spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you suggest to indent the text block?

@pascalgrimaud
Copy link
Member

another missing: you need to add the method in buildToolDomainService too

@swarajsaaj
Copy link
Contributor Author

another missing: you need to add the method in buildToolDomainService too

Thanks for pointing it out, added it.

plugin
.getAdditionalElements()
.ifPresent(additionalElements ->
result.append(plugin.getAdditionalElements().get().indent(4 * indentation).replaceAll("\n", System.lineSeparator()))
Copy link
Member

Choose a reason for hiding this comment

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

What happened if the initial text already contains \r\n? Will it become \r\r\n?

Copy link
Member

Choose a reason for hiding this comment

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

So if I understand well, text block with windows always use \n as Line separator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not just in windows, as per https://docs.oracle.com/en/java/javase/15/text-blocks/index.html#normalization-of-line-terminators , any line endings will become \n in a text block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would help to generate the code as per user's system line endings (in case jhipster-lite is generating on user's local)

Copy link
Member

Choose a reason for hiding this comment

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

ok thanks for explaination :)
Just to know, which OS do you use ?

I'm using Linux Ubuntu so it's hard for me to test Windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using Windows 10 as of now :)

Copy link
Member

Choose a reason for hiding this comment

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

perfect! so I'm totally trusting you for Windows part!

</includes>
</resource>
</webResources>
</plugin>""".replaceAll("\n", System.lineSeparator());
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about existing \r\n which could become \r\r\n

Co-authored-by: Pascal Grimaud <pascalgrimaud@gmail.com>
@pascalgrimaud pascalgrimaud merged commit ae46880 into jhipster:main Dec 21, 2021
@swarajsaaj
Copy link
Contributor Author

Thanks for the review and merge :)

@pascalgrimaud
Copy link
Member

Big thanks to you, for helping this project!

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.

2 participants