Code cleanup #1070

NewbieOrange · 2020-05-25T03:50:37Z

This PR removes redundant modifiers and initializations, unused variables, incorrect javadoc references, etc.

The static modifiers in inner interfaces are removed since they are static by default, and removing them should not break binary compatibility (the md5sums are the same). Let me know if those are preferred to be kept.

codecov-commenter · 2020-05-25T04:00:51Z

Codecov Report

Merging #1070 into master will increase coverage by 0.02%.
The diff coverage is 96.77%.

@@             Coverage Diff              @@
##             master    #1070      +/-   ##
============================================
+ Coverage     94.41%   94.43%   +0.02%     
  Complexity      448      448              
============================================
  Files             2        2              
  Lines          6591     6564      -27     
  Branches       1763     1765       +2     
============================================
- Hits           6223     6199      -24     
  Misses           94       94              
+ Partials        274      271       -3

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/picocli/CommandLine.java	`94.28% <96.72%> (+0.02%)`	`309.00 <7.00> (ø)`
src/main/java/picocli/AutoComplete.java	`96.98% <100.00%> (-0.06%)`	`139.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65256cc...701e48e. Read the comment docs.

NewbieOrange · 2020-05-25T04:06:27Z

Also there are a lot of @SuppressWarnings("deprecation") removed since we're at language level 1.5 and should not trigger any warnings.

remkop

Overall some nice improvements, thank you!
I put some comments, can you take a look?

remkop · 2020-05-25T07:04:50Z

src/main/java/picocli/CommandLine.java

@@ -7732,7 +7724,6 @@ void initFromMixin(UsageMessageSpec mixin, CommandSpec commandSpec) {
                if (initializable(sortOptions, mixin.sortOptions(), DEFAULT_SORT_OPTIONS))                             {sortOptions = mixin.sortOptions();}
                if (initializable(synopsisHeading, mixin.synopsisHeading(), DEFAULT_SYNOPSIS_HEADING))                 {synopsisHeading = mixin.synopsisHeading();}
                if (initializable(synopsisSubcommandLabel, mixin.synopsisSubcommandLabel(), DEFAULT_SYNOPSIS_SUBCOMMANDS)) {synopsisHeading = mixin.synopsisHeading();}
-                if (initializable(width, mixin.width(), DEFAULT_USAGE_WIDTH))                                          {width = mixin.width();}


Why is this removed?

This is never reached. initializable checks whether the varaible is null and is not the default value, but an int is never null.

private static boolean initializable(Object current, Object candidate, Object defaultValue) { return current == null && isNonDefault(candidate, defaultValue); }

If we want to keep this, we may introduce initializables with each primitive type.

This is a bug: #1072
Fixing this now.

Please revert this one; the warning that this condition is always false should go away with the latest master.

remkop · 2020-05-25T07:09:50Z

src/main/java/picocli/CommandLine.java

@@ -5138,9 +5133,8 @@ public Help create(CommandSpec commandSpec, Help.ColorScheme colorScheme) {
                return cls.cast(new LinkedHashMap<Object, Object>());
            }
            try {
-                @SuppressWarnings("deprecation") // Class.newInstance is deprecated in Java 9
-                T result = cls.newInstance();
-                return result;


This @SuppressWarnings("deprecation") annotation suppresses the warning one gets when building the project with Java 9. I think it is better to keep it.

remkop · 2020-05-25T07:10:18Z

src/main/java/picocli/AutoComplete.java

@@ -235,7 +235,7 @@ public void run() {
        }
    }

-    private static interface Function<T, V> {


No problem, I am okay with removing static for inner interfaces.

remkop · 2020-05-25T07:10:53Z

src/main/java/picocli/CommandLine.java

@@ -1470,7 +1470,7 @@ public void clearExecutionResults() {
     * @see RunAll
     * @deprecated Use {@link IExecutionStrategy} instead.
     * @since 2.0 */
-    @Deprecated public static interface IParseResultHandler {
+    @Deprecated public interface IParseResultHandler {


Again, no problem removing static for inner interfaces.

remkop · 2020-05-25T07:13:21Z

src/main/java/picocli/CommandLine.java

@@ -1758,7 +1758,7 @@ protected R throwOrExit(ExecutionException ex) {
     * @since 2.0 */
    @Deprecated public static class DefaultExceptionHandler<R> extends AbstractHandler<R, DefaultExceptionHandler<R>> implements IExceptionHandler, IExceptionHandler2<R> {
        public List<Object> handleException(ParameterException ex, PrintStream out, Help.Ansi ansi, String... args) {
-            internalHandleParseException(ex, new PrintWriter(out, true), Help.defaultColorScheme(ansi)); return Collections.<Object>emptyList(); }


I think I introduced this to get rid of a "raw collections" warning when building with Java 8 or higher... Probably better to keep the generified form with <Object>.

src/main/java/picocli/CommandLine.java

remkop · 2020-05-25T07:36:20Z

src/main/java/picocli/CommandLine.java

@@ -10038,7 +10025,6 @@ public MethodParam(Method method, int paramIndex) {
            }
            @Override public Annotation[] getDeclaredAnnotations() { return method.getParameterAnnotations()[paramIndex]; }
            @Override public void setAccessible(boolean flag) throws SecurityException { method.setAccessible(flag); }
-            @SuppressWarnings("deprecation")


Method.isAccessible will give deprecation warnings when building on Java 9+.
I believe we want to suppress this.
Try this:

%JAVA_11_HOME%\bin\javac -Xlint:all -d c:\Temp picocli\CommandLine.java

When I tried compiling on java 11 with -Xlint:all, I saw deprecation warnings for method.isAccessible.
You did not get this?

remkop · 2020-05-25T07:47:48Z

src/main/java/picocli/CommandLine.java

@@ -14943,7 +14921,7 @@ public IParamLabelRenderer createDefaultParamLabelRenderer() {
         *   OptionSpec#description()} array, and these rows look like {@code {"", "", "", option.description()[i]}}.</p>
         */
        static class DefaultOptionRenderer implements IOptionRenderer {
-            private String requiredMarker = " ";
+            private String requiredMarker;
            private boolean showDefaultValues;


Can we make requiredMarker and showDefaultValues final fields?

Can these be final too?

src/main/java/picocli/CommandLine.java

NewbieOrange · 2020-05-25T09:53:29Z

I have checked the other removed @SuppressWarnings does not introduce any new warning when compiled with OpenJDK 11.0.7 and -Xlint:all.

NewbieOrange · 2020-05-25T10:07:04Z

The added final modifiers seem to break the codegen graalvm-aot tests, since the generated fields have an entra "allowWrite" : true.

Edit: The field should not be made final, reverted.

NewbieOrange · 2020-05-25T10:23:36Z

On a side note, I think we should also remove the broken kloudtek logo (HTTPS cert expired) and machine-translated article (从Commons CLI迁移到Picocli) from README.

remkop · 2020-05-25T11:51:33Z

I have checked the other removed @SuppressWarnings does not introduce any new warning when compiled with OpenJDK 11.0.7 and -Xlint:all.

Thank you!

The added final modifiers seem to break the codegen graalvm-aot tests, since the generated fields have an entra "allowWrite" : true.

Edit: The field should not be made final, reverted.

Interesting! I’ll take a look tomorrow.

On a side note, I think we should also remove the broken kloudtek logo (HTTPS cert expired) and machine-translated article (从Commons CLI迁移到Picocli) from README.

Is the translation that bad? :-)
Even if it is, does it reflect badly on picocli?
I was thinking it’s better to have more links than fewer, to spread awareness of the library.

NewbieOrange · 2020-05-25T11:56:42Z

The added final modifiers seem to break the codegen graalvm-aot tests, since the generated fields have an entra "allowWrite" : true.
Edit: The field should not be made final, reverted.

Interesting! I’ll take a look tomorrow.

This is due to a private field being made final, but it is accessed by reflection from annotations. I have removed the final modifier that caused the issue.

On a side note, I think we should also remove the broken kloudtek logo (HTTPS cert expired) and machine-translated article (从Commons CLI迁移到Picocli) from README.

Is the translation that bad? :-)
Even if it is, does it reflect badly on picocli?
I was thinking it’s better to have more links than fewer, to spread awareness of the library.

The translation is simply google translate I think. It is not unreadable tho, but some code snippets are being 'translated', and it is also not the original post (it is a repost). The original post is here (the content is the same, but with better formatting): https://blog.csdn.net/Tybyqi/article/details/85787550

remkop

Please change the initFromMixin bit; other than that, looks good.

remkop · 2020-05-26T11:37:35Z

src/main/java/picocli/CommandLine.java

@@ -14943,7 +14921,7 @@ public IParamLabelRenderer createDefaultParamLabelRenderer() {
         *   OptionSpec#description()} array, and these rows look like {@code {"", "", "", option.description()[i]}}.</p>
         */
        static class DefaultOptionRenderer implements IOptionRenderer {
-            private String requiredMarker = " ";
+            private String requiredMarker;
            private boolean showDefaultValues;


Can these be final too?

remkop · 2020-05-26T11:39:09Z

src/main/java/picocli/CommandLine.java

@@ -10038,7 +10025,6 @@ public MethodParam(Method method, int paramIndex) {
            }
            @Override public Annotation[] getDeclaredAnnotations() { return method.getParameterAnnotations()[paramIndex]; }
            @Override public void setAccessible(boolean flag) throws SecurityException { method.setAccessible(flag); }
-            @SuppressWarnings("deprecation")


When I tried compiling on java 11 with -Xlint:all, I saw deprecation warnings for method.isAccessible.
You did not get this?

remkop · 2020-05-26T11:59:35Z

src/main/java/picocli/CommandLine.java

@@ -7732,7 +7724,6 @@ void initFromMixin(UsageMessageSpec mixin, CommandSpec commandSpec) {
                if (initializable(sortOptions, mixin.sortOptions(), DEFAULT_SORT_OPTIONS))                             {sortOptions = mixin.sortOptions();}
                if (initializable(synopsisHeading, mixin.synopsisHeading(), DEFAULT_SYNOPSIS_HEADING))                 {synopsisHeading = mixin.synopsisHeading();}
                if (initializable(synopsisSubcommandLabel, mixin.synopsisSubcommandLabel(), DEFAULT_SYNOPSIS_SUBCOMMANDS)) {synopsisHeading = mixin.synopsisHeading();}
-                if (initializable(width, mixin.width(), DEFAULT_USAGE_WIDTH))                                          {width = mixin.width();}


This is a bug: #1072
Fixing this now.

remkop · 2020-05-26T12:03:05Z

src/main/java/picocli/CommandLine.java

@@ -7732,7 +7724,6 @@ void initFromMixin(UsageMessageSpec mixin, CommandSpec commandSpec) {
                if (initializable(sortOptions, mixin.sortOptions(), DEFAULT_SORT_OPTIONS))                             {sortOptions = mixin.sortOptions();}
                if (initializable(synopsisHeading, mixin.synopsisHeading(), DEFAULT_SYNOPSIS_HEADING))                 {synopsisHeading = mixin.synopsisHeading();}
                if (initializable(synopsisSubcommandLabel, mixin.synopsisSubcommandLabel(), DEFAULT_SYNOPSIS_SUBCOMMANDS)) {synopsisHeading = mixin.synopsisHeading();}
-                if (initializable(width, mixin.width(), DEFAULT_USAGE_WIDTH))                                          {width = mixin.width();}


Please revert this one; the warning that this condition is always false should go away with the latest master.

NewbieOrange · 2020-05-26T12:12:03Z

Can these be final too?

Made the fields final.

When I tried compiling on java 11 with -Xlint:all, I saw deprecation warnings for method.isAccessible.
You did not get this?

No.. I think it should, but for some reason it is not giving me warning. I have added the @SupressWarnings annotation back.

Please revert this one; the warning that this condition is always false should go away with the latest master.

Done.

remkop · 2020-05-26T21:50:07Z

Merged into master.
Thank you for the contribution! Much appreciated!

NewbieOrange added 3 commits May 25, 2020 11:29

Various code cleanup and javadoc fixes

063c438

Various code cleanup

7610a4f

Restore code formatting

9c67506

remkop requested changes May 25, 2020

View reviewed changes

remkop added this to the 4.4 milestone May 25, 2020

Avoid warnings when compiled with -Xlint:all

2aad80b

Make final-able private fields final

bce4db2

NewbieOrange requested a review from remkop May 25, 2020 09:56

Revert making picocli.CommandLine.HelpCommand.commands final

bca2587

remkop requested changes May 26, 2020

View reviewed changes

Revert removing unreached initializable mixin width branch

5b2c3aa

NewbieOrange requested a review from remkop May 26, 2020 12:07

NewbieOrange added 2 commits May 26, 2020 20:07

Make more fields final

a271e2b

Add @SupressWarnings to isAccessible calls

701e48e

remkop merged commit 035f658 into remkop:master May 26, 2020

remkop added the type: enhancement ✨ label May 26, 2020

remkop added a commit that referenced this pull request May 26, 2020

[#1070] update RELEASE-NOTES for code cleanup

768ccbc

NewbieOrange deleted the code_cleanup branch May 27, 2020 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code cleanup #1070

Code cleanup #1070

NewbieOrange commented May 25, 2020

codecov-commenter commented May 25, 2020 •

edited

Loading

NewbieOrange commented May 25, 2020

remkop left a comment

remkop May 25, 2020

NewbieOrange May 25, 2020 •

edited

Loading

NewbieOrange May 25, 2020 •

edited

Loading

remkop May 26, 2020

remkop May 26, 2020

remkop May 25, 2020

remkop May 25, 2020

remkop May 25, 2020

remkop May 25, 2020

remkop May 25, 2020

remkop May 26, 2020

remkop May 25, 2020

remkop May 26, 2020

NewbieOrange commented May 25, 2020

NewbieOrange commented May 25, 2020 •

edited

Loading

NewbieOrange commented May 25, 2020

remkop commented May 25, 2020

NewbieOrange commented May 25, 2020 •

edited

Loading

remkop left a comment

remkop May 26, 2020

remkop May 26, 2020

remkop May 26, 2020

remkop May 26, 2020

NewbieOrange commented May 26, 2020 •

edited

Loading

remkop commented May 26, 2020

Code cleanup #1070

Code cleanup #1070

Conversation

NewbieOrange commented May 25, 2020

codecov-commenter commented May 25, 2020 • edited Loading

Codecov Report

NewbieOrange commented May 25, 2020

remkop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewbieOrange May 25, 2020 • edited Loading

Choose a reason for hiding this comment

NewbieOrange May 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewbieOrange commented May 25, 2020

NewbieOrange commented May 25, 2020 • edited Loading

NewbieOrange commented May 25, 2020

remkop commented May 25, 2020

NewbieOrange commented May 25, 2020 • edited Loading

remkop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewbieOrange commented May 26, 2020 • edited Loading

remkop commented May 26, 2020

codecov-commenter commented May 25, 2020 •

edited

Loading

NewbieOrange May 25, 2020 •

edited

Loading

NewbieOrange May 25, 2020 •

edited

Loading

NewbieOrange commented May 25, 2020 •

edited

Loading

NewbieOrange commented May 25, 2020 •

edited

Loading

NewbieOrange commented May 26, 2020 •

edited

Loading