pmd / pmd

An extensible multilanguage static code analyzer.
https://pmd.github.io
Other
4.9k stars 1.5k forks source link

[core] Support default CLI options and document usage examples with @-files and default files #2087

Open linusjf opened 5 years ago

linusjf commented 5 years ago

Affects PMD Version: All.

Rule: Not rule specific.

Description:

https://github.com/pmd/pmd/issues/2006#issuecomment-540282077

PMD needs a properties file to configure itself so that it can be run without specifying multiple switches on the command line. These switches should be configurable via the properties file and CLI with CLI overriding the files' values.

Running PMD through:

All.

oowekyala commented 5 years ago

It's already possible though not documented. It's a feature of JCommander:

JCommander supports the @ syntax, which allows you to put all your options into a file and pass this file as parameter

Eg write a file pmd_params with your parameters, one per line, then call pmd with pmd @path/to/pmd_params

linusjf commented 5 years ago

This looks promising. My concern is that this is parameter expansion as read from the file and may not allow overriding of switches /options. Does it, i.e., can I use the same parameters on the command line? e.g., can I do this given that I have -f -g in the file? java @pmdfile -f -g

I haven't got around to testing JCommander if it does support overriding. The documentation does not make any mention of it. I'm unable to find any CLI component that supports properties files. Would it be worthwhile to put in a request to JCommander to support properties files?

https://github.com/cbeust/jcommander/issues/330

adangel commented 5 years ago

I did a simple test:

This is the content of file pmd.parameters:

-d
pmd-core/src/main/java/net/sourceforge/pmd/cpd
-R
rulesets/java/quickstart.xml
-f
text

So, you specify each parameter in a extra line (also the options to a parameter are in a separate lines).

Then you can run pmd like this:

run.sh pmd @pmd.parameters

However, overriding is not possible - additional parameters are possible, but they are added additionally. Which makes sense, you specify it once via the file and once directly, so in sum - twice:

run.sh pmd @pmd.parameters -f xml
...
Can only specify option -f once.

the thus expanded @ file is added to the other commands (the present docu suggests replacing)

(from https://github.com/cbeust/jcommander/issues/330) So, this is true.

The question is: Do you need overriding? If you know, that one parameter (say e.g. the format) will vary, then just don't put it in the parameters file...

Btw.: CLI only supports specifying one format - the Ant task allows you to specify multiple formats (e.g. to create with one run a html and xml report). This is basically a lack of the CLI interface that PMD provides: e.g. format is not a List, so can be specified only once. But providing multiple format requires that we know for each format, where to save the report to. This means, that -reportfile must be a List as well. However, then we should maybe do something else, like specifying the format like -f text=report.txt -f xml=report.xml.

Btw.: your example doesn't make sense: -f needs an option (the format) and -g doesn't exist in PMD.

I'd suggest to improve the documentation to show, how @parameter files can be used.

If you have a concrete use case for overriding a specific parameter, then we should do this in a separate issue. It's better to solve a concrete problem than trying to fix unnecessary theoretical problems.

linusjf commented 5 years ago

The example was illustrative. I could have made it more relevant to PMD, I guess.

I don't have a specific use case in mind but precedent exists.

The best programs allow 'defaults' to be read from a properties or rc file (such as PerlCritic) permitting parameter-overriding from the command line.

The @ defeats the purpose of being able to specify defaults and forgetting about them until you need to update or override them. You will have to specify the @ on the command line every time unless you hard-code a default in the run.sh script and document it as such.

You have a point about PMD not really needing that ability, at least not yet. It helps that PMD doesn't have a Swiss-army knife utility-feel to it. Functionality such as CPD is separated out into a different java class.

I'd still like to be able to specify defaults in a file and then override them with only the overriding visible.

Then, there could be a switch that allows the user to specify a different properties file, if needed.

It would be best if this were part of its codebase, rather than PMD's.

linusjf commented 5 years ago

https://github.com/cbeust/jcommander/blob/master/src/main/java/com/beust/jcommander/JCommander.java

https://github.com/cbeust/jcommander/blob/master/src/main/java/com/beust/jcommander/defaultprovider/PropertyFileDefaultProvider.java

https://github.com/cbeust/jcommander/blob/master/src/main/java/com/beust/jcommander/IDefaultProvider.java

https://github.com/cbeust/jcommander/blob/master/src/test/java/com/beust/jcommander/DefaultProviderTest.java

I finally found the time to look at JCommander.

It appears that there is support for providing default values via a properties file.

The relevant sources are listed above.

However, it doesn't appear to support overriding. Does someone want to test this, though? It's the end of day for me here.

The documentation at jcommander.org makes no mention of property files, though.

This invalidates my comment above. Strikethrough?

I've raised an issue on JCommander : https://github.com/cbeust/jcommander/issues/479

linusjf commented 5 years ago

I tested the above for properties. And the property values are not being used.

https://github.com/Fernal73/LearnJava/tree/master/JCommander

I still think this should be part of JCommander's code base, not PMD's. Code can always be contributed to JCommander.

linusjf commented 5 years ago

As far as I can see, there have been no commits on JCommander for the last three months. Has the project been abandoned?

linusjf commented 5 years ago

Another option would be to check out picocli that Checkstyle uses.

https://picocli.info/#_default_provider

https://picocli.info/#_overwriting_single_options

Is it superior?

While it's still my opinion that Picocli and JCommander provide support for properties files fully, PMD might need to implement some of that functionality if it's not provided out-of-the-box.

https://github.com/remkop/picocli/issues/542

adangel commented 5 years ago

There is nothing preventing us from using an alternative for JCommander - it's just an implementation detail and opaque to the user.

But I'm still missing the use case... Why do we need that? Would it make PMD more complicated to use? E.g. if we allow overriding options, how easy/difficult is it to know, which options are now really used? Wouldn't that lead to other bugs, like "this option is not used", then we dig into it, and see, it is not used, because it is overridden later on...

linusjf commented 5 years ago

The initial usecase was to allow default properties to be set via profile files. As argued, this could be met via @files as well as long as single-valued parameters are not respecified via the command line. This would break JCommander.

My usecase is that sometimes I might wish to override these especially if I'm using a script that calls the java command (let's just say to test something different from my usual use). I don't wish to create another script that simply accepts "$@" which is simply a wrapper for the java command line class.

Why should I need two or more scripts when I could simply have multiple configurations?

oowekyala commented 5 years ago

Another option would be to check out picocli that Checkstyle uses.

IMO PicoCLI looks in general a bit more mature/feature-complete than JCommander. IIRC a really nice feature that JCommander does not do is generate autocompletion files for (at least) Bash. It's been a while since I looked into it though

especially if I'm using a script that calls the java command

This isn't clear to me. Is your script calling PMD directly using the java command, eg java $PMD_MAIN_CLASS?

Why don't you just copy the command-line and manually change options? It looks like you don't need to do this often, and that would work.

I don't wish to create another script that simply accepts "$@" which is simply a wrapper for the java command line class.

PMD's distribution already includes such scripts

if we allow overriding options, how easy/difficult is it to know, which options are now really used?

I think you're right that overridable options can be abused... but I would also think this is not directly PMD's concern, so does not really warrant emitting warnings. Though I'm pretty sure someone will cut themselves on it one day and feature-request that...

One other "problem" with overridable options is also that every switch should be negatable. For now we have default behaviour, and switches that enable the opposite of the default. There's no switch to go back to default behaviour. Eg if your default profile includes -debug, you can't override it with eg -no-debug later on.

It looks like allowing overriding CLI options would require a significant effort to implement, and especially to test. I'm not sure the effort is proportionate to the current use-case.

linusjf commented 5 years ago

I'm not using the PMD wrapper script. So, yes. Since PMD is not the only static analysis tool I use, I have my own script. I use static analysis as a learning tool to write better code. In practice, if you're using multiple ones, you'd be careful to run a rule only once in your project for efficiency. Projects test static analysis tools as a side effect. They're not written specifically for that.

Picocli has something called negatable options. That's neither here nor there.

Picocli has some elaborate stuff such as above built-in. I don't see why overridable options can't become standard for CLI APIs. It's quite common to have profiles for programs these days.

Different use cases would have to be captured. It can be argued that it then leaves very little for application developers to do themselves.

I'm not keen on implementing overridable options within PMD as well i.e., coding the thing. If you follow the picocli thread, though, there's a code sample how to do that provided by its creator.

Are you saying the users won't want them if you support profile files? They will when they're accustomed to using them elsewhere.

linusjf commented 5 years ago

https://github.com/remkop/picocli/releases/tag/v4.1.0

remkop commented 4 years ago

Hello all,

picocli has built-in support for a PropertiesDefaultProvider that I believe meets @Fernal73's requirements.

This is different from the @-file syntax: an option specified in an @-file and on the command line is applied twice, which may cause issues. By contrast, the default provider only provides a value if an option is not specified on the command line.

If the properties file does not exist, then the default values defined in the applications are applied. This gives end users a way to configure their own default values for the application's options. Users are free to use this or ignore this; this is simply a nice extra feature that mature CLI apps can provide to their users.

One other "problem" with overridable options is also that every switch should be negatable. For now we have default behaviour, and switches that enable the opposite of the default. There's no switch to go back to default behaviour. Eg if your default profile includes -debug, you can't override it with eg -no-debug later on.

I believe this is a side-effect of using @-files as a user-configurable source or default values. Picocli default providers do not suffer from this drawback. If the default value for --debug is false, then you could specify --no-debug on the command line and the command line value would be applied. (If you have many flags like this you may also be interested in picocli's support for negatable options.)

I hope this is useful.

linusjf commented 4 years ago

The decision to use picocli or not rests with the repo maintainers.

I think it's a good idea and can be migrated to in release 7.0.0.

Ps: I can't recall why I didn't like negatable options. Perhaps, it had to do with PMD only having two such options. Otherwise, they're pretty good to have.

adangel commented 4 years ago

Just FYI - the java command line supports this as well: https://docs.oracle.com/en/java/javase/14/docs/specs/man/java.html#java-command-line-argument-files

remkop commented 4 years ago

@adangel, regarding support for @-files (argument files): picocli also supports @-files, but looking at the discussion above, I believe picocli's support for a property-file based default provider may meet @Fernal73's requirements better.

Specifically, this supports overriding default values on the command line out of the box.

adangel commented 7 months ago

Since PMD 7.0.0 uses now picocli, the externalization of the parameters in a file should be possible: https://picocli.info/#AtFiles

All that is missing is an example in the documentation.

Update: we should also use https://picocli.info/#_propertiesdefaultprovider and also provide an example.