Skip to content

GEOWAVE-1731: update jcommander to v1.78#1732

Merged
rfecher merged 1 commit intolocationtech:masterfrom
rfecher:GEOWAVE-1731
May 28, 2020
Merged

GEOWAVE-1731: update jcommander to v1.78#1732
rfecher merged 1 commit intolocationtech:masterfrom
rfecher:GEOWAVE-1731

Conversation

@rfecher
Copy link
Copy Markdown
Contributor

@rfecher rfecher commented May 26, 2020

updated jcommander to v1.78

the major impact this had was no longer using a static instance of JCommander's Console which touched a lot of places.

While in "commands" I cleaned them and ensured that it didn't change the annotated parameters with "final" by adding setters (generally have adopted that as the best practice to avoid cleanup impacting functionality because JCommander uses reflection it will throw exceptions for inaccessible fields if they are final).

Also renamed the few places that I came across that still have "Geowave" when it should be "GeoWave" consistently across the project for class names.

@rfecher rfecher requested a review from jdgarrett May 26, 2020 15:20
@rfecher rfecher force-pushed the GEOWAVE-1731 branch 4 times, most recently from 64328e2 to 3254f18 Compare May 26, 2020 20:23
Signed-off-by: Rich Fecher <rich.fecher@blacklynx.tech>
Copy link
Copy Markdown
Contributor

@jdgarrett jdgarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. At some point I think we will have to go to a custom Console interface so that we aren't tied to JCommander outside of classes that are specifically for CLI commands. I've noticed a lot of places that directly use System.out or System.err when they should really be using the console.

@rfecher rfecher merged commit 675781d into locationtech:master May 28, 2020
@rfecher rfecher deleted the GEOWAVE-1731 branch May 28, 2020 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants