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

support context variables at PackageJson and Content Replacer API #9758

Conversation

renanfranca
Copy link
Contributor

Fix #9744

@renanfranca
Copy link
Contributor Author

I still in draft because I am going to update the modules in this pull request too 🖖

@renanfranca
Copy link
Contributor Author

I identified the sonar bug, and I will fix it later 👍

@renanfranca renanfranca force-pushed the 9744-replace-hard-coded-target-path-in-package-json-with-context-value branch 2 times, most recently from 6f1ba5a to efde02c Compare May 9, 2024 11:10
@renanfranca
Copy link
Contributor Author

I encounter a new issue when trying to replace this target for {{projectBuildDirectory}} at AngularModuleFactory and it didn't work because it is inserted with replacer. After all, pckageJson API does not support jestSonar block.

I see two possibilities:

  1. add support for jestSonar block
  2. add support for replacer handle context variable and update it.

I am going to try to implement the second option because this jestSonar block is only needed at AngularModuleFactory and is not generic enough to deserve an implementation at packageJson API.

@renanfranca renanfranca marked this pull request as ready for review May 9, 2024 16:17
@renanfranca
Copy link
Contributor Author

@murdos : I implemented add support for replacer handle context variable and update it. Depending on your review I could refactor the packageJson API. Thanks in advance and take your time 😉🖖

@renanfranca renanfranca requested a review from murdos May 9, 2024 16:24
@@ -0,0 +1,10 @@
package tech.jhipster.lite.module.domain.packagejson;

record PackageJsonData(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really not found of this (and is the reason why I've not approved this PR earlier). It brings some unneeded coupling.
If Sonar is not happy about duplication for these pieces of code, we can disable the rule locally with a @SuppressWarning.

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'm really not found of this (and is the reason why I've not approved this PR earlier). It brings some unneeded coupling.

I can't agree more! It was only to make sonar let me go.

If Sonar is not happy about duplication for these pieces of code, we can disable the rule locally with a @SuppressWarning.

That's great news!!! I am going to revert this change and add the supresswarning!!!

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 remove the need for PackageJsonChanges.java using the context from JHipsterModuleChanges. This will also address the code duplication issue identified by Sonar as a secondary benefit.

BTW: I didn't find a way to ignore sonar code duplication using @SuppressWarning. I am curious if I didn't search enough, I would like to know if you know how 😃

@renanfranca renanfranca force-pushed the 9744-replace-hard-coded-target-path-in-package-json-with-context-value branch from 13ab437 to 5bfbc94 Compare May 11, 2024 00:55
@renanfranca renanfranca requested a review from murdos May 11, 2024 01:08
@renanfranca renanfranca changed the title support context variables at packageJson API support context variables at packageJson and Content Replacer API May 11, 2024
@renanfranca renanfranca force-pushed the 9744-replace-hard-coded-target-path-in-package-json-with-context-value branch from 912683e to 092d563 Compare May 11, 2024 01:17
@renanfranca renanfranca changed the title support context variables at packageJson and Content Replacer API support context variables at PackageJson and Content Replacer API May 11, 2024
@murdos murdos merged commit a10f84d into jhipster:main May 11, 2024
35 checks passed
@renanfranca renanfranca deleted the 9744-replace-hard-coded-target-path-in-package-json-with-context-value branch May 11, 2024 09:13
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.

Replace hard coded target/ path in package.json using projectBuildDirectory context value
2 participants