[core] Backport to PMD6 changes to accomodate PicoCli in PMD7 by jsotuyod · Pull Request #4161 · pmd/pmd

jsotuyod · 2022-10-15T02:40:03Z

Describe the PR

Companion PR for [core] Use PicoCli and unify PMD usage under a single main #4059
Add deprecations and path the way for the upcoming changes to the CLI in PMD7
Cover the PMD6 side of [core] Deprecate -language, -version, -min options #3426

Related issues

Fixes [core] Deprecate -language, -version, -min options #3426

Ready?

Added unit tests for fixed bug/feature
Passing all unit tests
Complete build ./mvnw clean verify passes (checked automatically by github actions)
Added (in-code) documentation (if needed)

- Companion PR for pmd#4059 - Add deprecations and path the way for the upcoming changes to the CLI in PMD7

ghost · 2022-10-15T03:27:12Z

	1 Message
📖	No regression tested rules have been changed.
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report

Generated by 🚫 Danger

adangel

Thanks!

I noticed recently, we use "-stress" in PMDCoverageTest, but I think it's really not needed.

"-benchmark" is used in the same test class. But it is also used in maven-pmd-plugin. Do we need to change anything on this side? (probably not for pmd6, but for pmd7?)

adangel · 2022-10-16T08:47:35Z

        m.put("-no-cache", "--no-cache");
+        m.put("-v", "--use-version"); // In PMD 7, -v will enable verbose mode
+        m.put("-V", "--verbose"); // In PMD 7, -V will show the tool version
+        m.put("-min", "--minimum-priority");


I was just wondering about deprecating "-min", but it actually fixes #3426 👍

adangel · 2022-10-16T08:49:52Z

    readonly CLASSNAME="net.sourceforge.pmd.util.viewer.Viewer"
    ;;
  "cpdgui")
+    echo "'cpdgui' is deprecated and will be removed in PMD 7.0.0, use 'cpd-gui' instead."


pmd cpd-gui
This command is not working in Command Prompt. It just open window for a moment then automatically close.

@VishV-Android please, open a new issue with full details.

@jsotuyod #4913 (comment)
New issue created with details.

jsotuyod · 2022-10-17T02:23:59Z

I noticed recently, we use "-stress" in PMDCoverageTest, but I think it's really not needed.

It's not. The PMDConfiguration.isStressTest() method is never called within PMD7, it's been a noop for some time. In PMD6 the behavior is moot (shuffle input files). The whole test however is to be removed from PMD7 once we remove the old CLI code (as it would fail to run).

"-benchmark" is used in the same test class. But it is also used in maven-pmd-plugin. Do we need to change anything on this side? (probably not for pmd6, but for pmd7?)

Benchmark is useful (does print timing report to stderr), but it's now implemented fully at pmd-cli in PMD7… I did so because:

it's not structured output, so it's intended for a human reading it
STDERR is pretty much a CLI concern

I didn't think about Maven honestly… Maven would not be able to pass the flag (it can call the rendering itself though), but does it make sense? is there really a case for this flag existing on Maven? Is this flag useful to anyone else beyond ourselves to measure the impact of changes we do to PMD?

adangel · 2022-10-18T17:26:30Z

I noticed recently, we use "-stress" in PMDCoverageTest, but I think it's really not needed.

It's not. The PMDConfiguration.isStressTest() method is never called within PMD7, it's been a noop for some time. In PMD6 the behavior is moot (shuffle input files). The whole test however is to be removed from PMD7 once we remove the old CLI code (as it would fail to run).

👍

"-benchmark" is used in the same test class. But it is also used in maven-pmd-plugin. Do we need to change anything on this side? (probably not for pmd6, but for pmd7?)

Benchmark is useful (does print timing report to stderr), but it's now implemented fully at pmd-cli in PMD7… I did so because:
* it's not structured output, so it's intended for a human reading it

* STDERR is pretty much a CLI concern
I didn't think about Maven honestly… Maven would not be able to pass the flag (it can call the rendering itself though), but does it make sense? is there really a case for this flag existing on Maven? Is this flag useful to anyone else beyond ourselves to measure the impact of changes we do to PMD?

I found the original feature request here: https://issues.apache.org/jira/browse/MPMD-181
So, it seems to be useful for analyzing slow builds.
I think, we should consider this again later and not now - once pmd7 is closer to release. We have a separate pmd7 branch for the maven-pmd-plugin (https://github.com/apache/maven-pmd-plugin/tree/pmd7) where we can adjust the plugin to be compatible with pmd7.

adangel · 2022-10-27T13:39:50Z

@jsotuyod This PR looks ready - except for the refactoring around CPD / CpdAnalysis. Maybe it's better to do this in a separate PR, that way we can bring in this PR already with 6.51.0 and continue with #4059 in pmd7. Wdyt?

oowekyala

This looks ready to me as well. I agree implementing this CpdAnalysis would be a better fit for another PR. If we merge this as is, we can also move forward with #4059.

Co-authored-by: Clément Fournier <clement.fournier@tu-dresden.de>

jsotuyod

Typo

oowekyala

A couple of comments that came to me when using PMDConfiguration in PMD 7. I will submit a PR for PMD 6.52.0

oowekyala · 2022-11-16T18:27:56Z

+     *
+     * @return URI
+     */
+    public URI getUri() {


I think this name is too vague and we should at least rename it (eg to getSourceDbUri or so)... Maybe it's also the right time to deprecate this feature altogether. It's untested, and it's completely unclear how to use it at all.

This is/was actually a PMD 7 misc task (in #2524). I've created a separate issue for this for pmd 7 -> #4216

Since this feature exists already, we should document how to use it in any case.
Remaining question is: Should we remove it with pmd 7?

oowekyala · 2022-11-16T18:33:00Z

+        return inputFilePath == null ? null : inputFilePath.toString();
+    }
+
+    public Path getInputFile() {


Similarly, this is very vague and confusing given we have getInputPathList and other similar methods. I would name this getInputFileListPath maybe

oowekyala · 2022-11-16T18:33:18Z

+        return ignoreFilePath == null ? null : ignoreFilePath.toString();
+    }
+
+    public Path getIgnoreFile() {


getIgnoreFileListPath

Backport to PMD6 changes to accomodate PicoCli in PMD7

b9737e2

- Companion PR for pmd#4059 - Add deprecations and path the way for the upcoming changes to the CLI in PMD7

jsotuyod mentioned this pull request Oct 15, 2022

[core] Use PicoCli and unify PMD usage under a single main #4059

Merged

16 tasks

Avoid NPEs

7d0208f

adangel reviewed Oct 16, 2022

View reviewed changes

jsotuyod added 4 commits October 17, 2022 00:45

Include CLI deprecations

9e92ca3

Add deprecations

21306f3

Fix link to JavaFX

637b701

Update CLI docs

ecd7ab1

adangel modified the milestones: 6.51.0, 6.52.0 Oct 27, 2022

oowekyala approved these changes Nov 2, 2022

View reviewed changes

Comment thread docs/pages/pmd/userdocs/cli_reference.md Outdated

Update docs/pages/pmd/userdocs/cli_reference.md

6c5c34b

Co-authored-by: Clément Fournier <clement.fournier@tu-dresden.de>

jsotuyod marked this pull request as ready for review November 10, 2022 15:28

jsotuyod commented Nov 10, 2022

View reviewed changes

Comment thread docs/pages/release_notes.md Outdated

Fix typo

c513d72

jsotuyod merged commit 79e294b into pmd:master Nov 10, 2022

jsotuyod deleted the prep-4059 branch November 10, 2022 15:34

oowekyala reviewed Nov 16, 2022

View reviewed changes

Uh oh!

Conversation

jsotuyod commented Oct 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the PR

Related issues

Ready?

Uh oh!

ghost commented Oct 15, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adangel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adangel Oct 16, 2022

Choose a reason for hiding this comment

Uh oh!

adangel Oct 16, 2022

Choose a reason for hiding this comment

Uh oh!

VishV-Android Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsotuyod Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

VishV-Android Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsotuyod commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adangel commented Oct 18, 2022

Uh oh!

adangel commented Oct 27, 2022

Uh oh!

oowekyala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsotuyod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oowekyala left a comment

Choose a reason for hiding this comment

Uh oh!

oowekyala Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

adangel Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

oowekyala Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

oowekyala Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsotuyod commented Oct 15, 2022 •

edited

Loading

ghost commented Oct 15, 2022 •

edited by ghost

Loading

VishV-Android Apr 3, 2024 •

edited

Loading

VishV-Android Apr 3, 2024 •

edited

Loading

jsotuyod commented Oct 17, 2022 •

edited

Loading