Conversation
Coverage Report for CI Build 34821751469Coverage decreased (-0.02%) to 73.886%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions29 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
2bfbd26 to
6f7eddb
Compare
|
thank you @laert-ll for your work on this there a some point where i see some improvments so let me know if you have any further Question on this |
| try { | ||
| currentVersion = buildTool.getProjectVersion(projectPath); | ||
| } catch (UnsupportedOperationException e) { | ||
| throw new CliException("The build tool " + buildTool.getName() + " detected in " + projectPath + " does not support releasing."); |
There was a problem hiding this comment.
The caught exception is lost here. Per documentation/contributing/coding-conventions.adoc ("Catching and handling Exceptions"), when wrapping and re-throwing the caught exception must be passed as the cause:
throw new CliException("...", e);
| */ | ||
| public interface BuildTool { | ||
|
|
||
| Logger LOG = LoggerFactory.getLogger(BuildTool.class); |
There was a problem hiding this comment.
Having a Logger field in this interface is a bit unusual. the coding conventions ("Logging") recommend using this.context instead of a LOG field for commandlets. If LOG is only used by the findWrapper default method, could we log via the passed-in IdeContext instead?
| default VersionIdentifier getProjectVersion(Path projectPath) { | ||
|
|
||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the {@link #getProjectVersion(Path) version} of the project. | ||
| * | ||
| * @param projectPath the {@link Path} to the top-level directory of the project. | ||
| * @param version the new {@link VersionIdentifier version} to set. | ||
| */ | ||
| void setProjectVersion(Path projectPath, VersionIdentifier version); | ||
| default void setProjectVersion(Path projectPath, VersionIdentifier version) { | ||
|
|
||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * Performs a single build-and-deploy (release) build of the project in the current working directory. | ||
| * | ||
| * @param additionalArgs the additional arguments to append to the build command (may be {@link List#isEmpty() empty}). | ||
| * @return the {@link ProcessResult} of the build (allowing the caller to react on {@link ProcessResult#isSuccessful() failures}, e.g. by retrying). | ||
| */ | ||
| ProcessResult buildAndDeploy(List<String> additionalArgs); | ||
| default ProcessResult buildAndDeploy(List<String> additionalArgs) { | ||
|
|
||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
This is allowed by the issue, but using UnsupportedOperationException as a capability flag is exception-as-control-flow (callers must catch it). For a follow-up, a nullable query (e.g. tryGetProjectVersion) or an explicit capability check would read more naturally.
| super(); | ||
| this.context = context; | ||
| this.commandletTypeMap = new HashMap<>(); | ||
| this.commandletTypeMap = new LinkedHashMap<>(); |
There was a problem hiding this comment.
With HashMap → LinkedHashMap, the iteration order of getCommandlets() / commandletIterator now changes globally. this also affects command completion and findCommandlet. I assume the insertion order is required for the Yarn-before-Npm priority, but it is a side effect beyond the build tools. Also, the "Yarn must be registered before Npm" constraint now only lives in the CommandletManager javadoc. a short comment at the registration site (add(new Yarn(...)); add(new Npm(...));) would keep it visible where it is enforced.
| protected void configureToolBinary(ProcessContext pc, ProcessMode processMode) { | ||
| Path gradle = Path.of(getBinaryName()); | ||
| Path wrapper = findWrapper(GRADLE_WRAPPER_FILENAME); | ||
| Path wrapper = findWrapper(this.context.getCwd(), GRADLE_WRAPPER_FILENAME); |
There was a problem hiding this comment.
With the new interface default, doesn't this override become a no-op (return null;)? If it adds nothing, it could be removed.
3 participants
This PR fixes #2420
Implemented changes:
CommandletManager.findBuildToolnow returnsBuildToolinstead ofLocalToolCommandlet.BUILD_TOOLSlist inCommandletManagerImplwith dynamic iteration over all commandlets usinginstanceof BuildToolpattern matching.commandletTypeMapfromHashMaptoLinkedHashMapand movedNpmregistration afterYarnso registration order decides detection priority (YarnbeforeNpm).findBuildDescriptorandfindWrapperfromLocalToolCommandlet(moved toBuildTool)Gradle,Npm, andYarnimplementBuildTool;Yarn.findBuildDescriptoronly matches whenyarn.lockis present and falls back toNpmotherwise.BuildCommandletto useBuildTooldirectly instead of casting throughLocalToolCommandlet.ReleaseCommandletto catchUnsupportedOperationExceptionfromgetProjectVersion/setProjectVersionand rethrow as aCliExceptionwith a clear "does not support releasing" message.Testing instructions
Test projects for points 1 - 5 can be found under
cli/src/test/resources/ide-projects/build/project/workspaces/main/and for points 6 - 8 undercli/src/test/resources/ide-projects/release/project/workspaces/main/.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc