Feature/new cli library #154

pesse · 2019-06-11T21:45:53Z

Introduces Picocli as new command-line library

Structures the options and organizes them into immutable configuration objects which can better be computed and shared.

Fixes #143

Introduced new Config-Objects to abstract away cli-arguments to config transformation. Will also be needed for YAML/JSON configuration in the future

Also fixed a bug - the Object types are the actual key which should be translated into Key-Values later

No need for RunActionTest anymore

Pazus

I guess we have some small behaviour changes, should we document them in the readme?

src/main/java/org/utplsql/cli/ICommand.java

src/main/java/org/utplsql/cli/ConnectionInfo.java

src/main/java/org/utplsql/cli/RunAction.java

Pazus · 2019-06-18T06:47:27Z

src/main/java/org/utplsql/cli/ReporterManager.java


 class ReporterManager {

    private List<ReporterOptions> reporterOptionsList;
    private List<Throwable> reporterGatherErrors;
    private ExecutorService executorService;

-    ReporterManager(List<String> reporterParams ) {
-        initReporterOptionsList(reporterParams);
+    ReporterManager(ReporterConfig[] reporterConfigs ) {


Let me share with you my point. I prefere to use simple constructors and use factory methods for all necessary operation neede to call the constructor. I don't like calling functions from a constructor as it doesn't allow making fields final and I personnaly like final fields. Do you mind making a small refactoring of this place?

You mean like

public static ReporterManager create( ReporterConfig[] reporterConfigs ) { ...

?

yes with all necessary stuff populating arrays done there and then call a simple private constructor.

src/main/java/org/utplsql/cli/ReporterManager.java

Pazus · 2019-06-18T06:54:22Z

src/main/java/org/utplsql/cli/ReporterManager.java

@@ -108,38 +112,58 @@ void startReporterGatherers(ExecutorService executorService, final DataSource da

        this.executorService = executorService;


Should we move it to the constructor? don't we know the executor service at construction time? That way we could make it also final.

ExecutorService is added later, but maybe this can be refactored, too.
I'd like to postpone this to the restructuring of ReporterManager

src/main/java/org/utplsql/cli/ReporterManager.java

Pazus · 2019-06-18T06:58:18Z

src/main/java/org/utplsql/cli/ReporterManager.java

+                }
+
+                option.getReporterObj().getOutputBuffer().printAvailable(conn, printStreams);
+            } catch (SQLException | FileNotFoundException e) {


Why don't we fall on any exception but only on these two?

Pazus · 2019-06-18T07:00:49Z

src/main/java/org/utplsql/cli/ReporterOptions.java

@@ -14,15 +14,15 @@

    private Reporter reporterObj = null;

-    public ReporterOptions(String reporterName, String outputFileName, boolean outputToScreen) {
+    public ReporterOptions(String reporterName, String outputFileName ) {
        setReporterName(reporterName);


These two fields should be final and set directly, not through the setters.

And check what fields can also be final, seems like first three.

Pazus · 2019-06-18T07:05:37Z

src/main/java/org/utplsql/cli/RunAction.java

+        int returnCode = 0;
+        try {
+
+            final List<Reporter> reporterList;


Could you please move it to initialization.

Pazus · 2019-06-18T07:12:18Z

dbout for example

pesse · 2019-06-18T07:24:54Z

I don't like the current state of ReporterManager and ReporterOptions at all. Much fuzz, many unnecessary things, not immutable etc.
@Pazus would you be okay with me refactoring these two in a separate PR? I want to move more towards the new ReporterConfig class (which you should like far better) but did not have time to do it properly yet.

Pazus · 2019-06-18T09:17:48Z

I don't like the current state of ReporterManager and ReporterOptions at all. Much fuzz, many unnecessary things, not immutable etc.
@Pazus would you be okay with me refactoring these two in a separate PR? I want to move more towards the new ReporterConfig class (which you should like far better) but did not have time to do it properly yet.

ok!

Pazus · 2019-06-18T09:19:35Z

src/main/java/org/utplsql/cli/ReporterManager.java

+    private ReporterOptions getDefaultReporterOption() {
+        return new ReporterOptions(CoreReporters.UT_DOCUMENTATION_REPORTER.name());
+    }
+
    private void abortGathering(Throwable e) {
        addGatherError(e);
        executorService.shutdownNow();


I realized this might be an issue. We get the executorService from the outside but we shut it down here. I'm concerned if the caller is ok with its executorService been shut down.

That's actually the purpose: If one Reporter Consumption fails, it should abort all the other Reporter threads.
But that's one of the things I want to do differently/more clear

lets abort tasks then, not the excutor

Yes, will make that more clear in a restructured version

pesse · 2019-06-26T06:47:39Z

Requested changes will come in a later iteration

pesse added 25 commits June 5, 2019 00:13

Failing test to catch and reproduce #143

6717057

Merge branch 'develop' into feature/new_cli_library

038981f

Unit-Test to simulate passing all parameters that should be known

b21f651

Initial implementation of Picocli usage

ce849fc

Introduced new Config-Objects to abstract away cli-arguments to config transformation. Will also be needed for YAML/JSON configuration in the future

Some more tests around FileMapping

5520562

Also fixed a bug - the Object types are the actual key which should be translated into Key-Values later

New RunAction which does the logic based on RunCommandConfig

f9c4cf7

RunAction tested and functional so far

d8cbce2

Include RunAction into RunCommand

3b5e168

New entry point for Picocli

60997d7

Extract functionality to interface

d13ae5b

Adapt tests to IRunCommand and implement necessary functionality

5e86b01

Change VersionCommand to Picocli

00f3f5f

Change ReportersCommand to Picocli

2ce4e91

Fix arity for mapping options and prevent NPE

b88d5c5

Refactor RunCommandTest to cover new approach

c80dc8c

No need for RunActionTest anymore

Implement rules for printToScreen Reporters: Only one can go to screen

8eec2cb

Implemented new Picocli help

ded39d3

Removed some now unnecessary stuff

6922e52

We don't need the old RunCommand anymore, time to get rid of it

5aa4c1d

Improve help and help-tests

60fcfaf

Refactor: Remove unnecessary logic

a3608b5

Refactor: extract ReporterConfig -> ReporterOption conversion

09bf357

Refactor: getCommand() no longer needed

1b65b51

Refactor: Include only left check-functionality

77fd752

Refactor: Cleanup ReporterManager a bit

e443e3c

pesse marked this pull request as ready for review June 13, 2019 21:51

pesse requested a review from Pazus June 13, 2019 21:51

pesse changed the title ~~[WIP] Feature/new cli library~~ Feature/new cli library Jun 13, 2019

Get rid of JCommander dependency

0bdca7d

Pazus requested changes Jun 18, 2019

View reviewed changes

Pazus reviewed Jun 18, 2019

View reviewed changes

pesse added 7 commits June 18, 2019 23:47

Move ConnectionString description to main Utplsql-Command

ae0e9c4

Fix Typo

f47c0de

Some Refactorings and make things final

bb18ea1

Merge branch 'develop' into feature/new_cli_library

1a984ff

Reformatting

81e0953

Add -h option to all commands and cover it with tests

a8acbb9

Adjust documentation to reflect parameter changes

10bef7b

pesse merged commit 7725326 into develop Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/new cli library #154

Feature/new cli library #154

pesse commented Jun 11, 2019 •

edited

Loading

Pazus left a comment

Pazus Jun 18, 2019

pesse Jun 18, 2019

Pazus Jun 18, 2019

Pazus Jun 18, 2019

pesse Jun 18, 2019

Pazus Jun 18, 2019

Pazus Jun 18, 2019

Pazus Jun 18, 2019

Pazus Jun 18, 2019

Pazus Jun 18, 2019

Pazus commented Jun 18, 2019

pesse commented Jun 18, 2019

Pazus commented Jun 18, 2019

Pazus Jun 18, 2019

pesse Jun 18, 2019

Pazus Jun 19, 2019

pesse Jun 19, 2019

pesse commented Jun 26, 2019

		@@ -108,38 +112,58 @@ void startReporterGatherers(ExecutorService executorService, final DataSource da

		this.executorService = executorService;

Feature/new cli library #154

Feature/new cli library #154

Conversation

pesse commented Jun 11, 2019 • edited Loading

Pazus 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

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

Pazus commented Jun 18, 2019

pesse commented Jun 18, 2019

Pazus commented Jun 18, 2019

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

pesse commented Jun 26, 2019

pesse commented Jun 11, 2019 •

edited

Loading