Revert disable.Pnpm logic #7405
Conversation
project-bot
bot
added this to External Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
There was a problem hiding this comment.
Reviewed 1 of 18 files at r1.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
a discussion (no related file):
As discussed with @mstahv , it might be good to split this change into two phases:
- rename the parameter (this definitely needs to be done)
- switch the default from
pnpmtonpm.
This is due to it being still debated if we make the switch or not.
Also the 1) should be picked to master branch.
flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildFrontendMojo.java, line 137 at r1 (raw file):
pnpmEnable
should this be pnpm.enable instead ? Can it refer to value in Contants.java:
public static final String SERVLET_PARAMETER_ENABLE_PNPM = "pnpm.enable";
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
a discussion (no related file):
Previously, pleku (Pekka Hyvönen) wrote…
As discussed with @mstahv , it might be good to split this change into two phases:
- rename the parameter (this definitely needs to be done)
- switch the default from
pnpmtonpm.This is due to it being still debated if we make the switch or not.
Also the 1) should be picked tomasterbranch.
Sorry I should have realized the split before, but just didn't. The parameter needs to be renamed in v15 too and thus it should go to master.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @pleku)
flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildFrontendMojo.java, line 137 at r1 (raw file):
Previously, pleku (Pekka Hyvönen) wrote…
pnpmEnableshould this be
pnpm.enableinstead ? Can it refer to value inContants.java:public static final String SERVLET_PARAMETER_ENABLE_PNPM = "pnpm.enable";
Tried that.
Maven doesn't accept dots in properties.
Or I don't know how to make it properly.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildFrontendMojo.java, line 137 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
Tried that.
Maven doesn't accept dots in properties.
Or I don't know how to make it properly.
Ok, I don't understand how it was then disable.pnpm before. Maybe it didn't even work before ?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildFrontendMojo.java, line 137 at r1 (raw file):
Previously, pleku (Pekka Hyvönen) wrote…
Ok, I don't understand how it was then
disable.pnpmbefore. Maybe it didn't even work before ?
:(((((
My memory is terrible.
property name may contain dots. You are right.
The parameter name can't contain dots (java name can't contain of course but there is away to
use name in the annotation and the name can't contain dots).
I'm confusing the property name and the parameter name.
Right. Will change the property name to one with dot.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @mstahv)
a discussion (no related file):
Previously, pleku (Pekka Hyvönen) wrote…
Sorry I should have realized the split before, but just didn't. The parameter needs to be renamed in v15 too and thus it should go to master.
OK, then it's good that I've created the PR in the master branch by mistake.
There was a problem hiding this comment.
Reviewed 12 of 18 files at r1, 5 of 6 files at r2.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @mstahv)
flow-migration/src/main/java/com/vaadin/flow/migration/MigrationConfiguration.java, line 351 at r2 (raw file):
public void setEnablePnpm(boolean enable) {
While this is now 1-1 with the property name, it doesn't match the getter.
I would rename this and the to setPnpmEnabled(boolean enabled) and similarly the property. Alternative would be to rename the getter to match this setter isEnablePnpm(), but because it is horrible, I would use the first.
flow-server/src/main/java/com/vaadin/flow/server/Constants.java, line 220 at r2 (raw file):
/** * Configuration parameter name for enabling pnpm. */
BTW this is missing an empty since tag (could even use since 2.2 now)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @pleku)
flow-migration/src/main/java/com/vaadin/flow/migration/MigrationConfiguration.java, line 351 at r2 (raw file):
Previously, pleku (Pekka Hyvönen) wrote…
public void setEnablePnpm(boolean enable) {While this is now 1-1 with the property name, it doesn't match the getter.
I would rename this and the to
setPnpmEnabled(boolean enabled)and similarly the property. Alternative would be to rename the getter to match this setterisEnablePnpm(), but because it is horrible, I would use the first.
Done.
setPnpmEnabled : OK
property: pnpmEnabled I would say this is a bad parameter name.
It's used in the pom file as <pnpmEnabled> and I think <pnpmEnable>true</pnpmEnable> is more suitable.
But I can change it it you think it should be changed.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
|
SonarQube analysis reported 11 issues
Top 10 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
denis-anisimov
moved this from Done - pending release
to Released
in OLD Vaadin Flow ongoing work (Vaadin 10+)
Fixes #7398
This change is