New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ease the creation of others sub-commands for cli #266

Merged
merged 1 commit into from May 21, 2015

Conversation

Projects
None yet
3 participants
@rlespinasse

rlespinasse commented May 19, 2015

Internally the cli configuration is done with the Service Provider API.

Now, all classes who implements CliCommand and are declared as implementation for the service provider API are added to the cli automatically.

All current sub-commands implements CliCommand.

Ease the creation of others sub-commands for cli
Internally the cli configuration is done with the Service Provider API.
Now, all classes who implements `CliCommand` and are declared as
implementation for the service provider API are added to the cli automatically.
All current sub-commands implements `CliCommand`.
JCommander parsedJCommander = cmd.getCommands().get(parsedCommand);
Object commandObject = parsedJCommander.getObjects().get(0);
if(commandObject instanceof CliCommand) {
((CliCommand) commandObject).execute();

This comment has been minimized.

@jponge

jponge May 19, 2015

Member

Isn't there a variant with generics?

This comment has been minimized.

This comment has been minimized.

@jponge

jponge May 19, 2015

Member

That's type unsafe :-)

On 19 May 2015, at 15:32, Romain Lespinasse notifications@github.com wrote:

In src/main/java/fr/insalyon/citi/golo/cli/Main.java:

  •      case "diagnose":
    
  •        diagnose(diagnose);
    
  •        break;
    
  •      case "doc":
    
  •        doc(doc);
    
  •        break;
    
  •      case "new":
    
  •        init(init);
    
  •        break;
    
  •      default:
    
  •        throw new AssertionError("WTF?");
    
  •    String parsedCommand = cmd.getParsedCommand();
    
  •    JCommander parsedJCommander = cmd.getCommands().get(parsedCommand);
    
  •    Object commandObject = parsedJCommander.getObjects().get(0);
    
  •    if(commandObject instanceof CliCommand) {
    
  •      ((CliCommand) commandObject).execute();
    
    JCommander object love Object as generic type
    https://github.com/cbeust/jcommander/blob/master/src/main/java/com/beust/jcommander/JCommander.java#L1451-L1453


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@rlespinasse

rlespinasse May 19, 2015

With that, no need of

@SuppressWarnings("unchecked")

everywhere 😃

@jponge jponge added this to the 3.0.0 milestone May 19, 2015

@jponge jponge added the improvement label May 19, 2015

@jponge jponge self-assigned this May 19, 2015

@jponge

This comment has been minimized.

Member

jponge commented May 19, 2015

Looks good to me. Ping @danielpetisme

@yloiseau

This comment has been minimized.

Contributor

yloiseau commented May 19, 2015

no comment... but I see conflicts comming 😓

@rlespinasse

This comment has been minimized.

rlespinasse commented May 19, 2015

This PR can wait, and I can deal with the conflicts 😄, can you tell me which PR can be in conflict?

@yloiseau

This comment has been minimized.

Contributor

yloiseau commented May 19, 2015

Not yet in PR but in my macro wip branch, but I don't think it will be. too hard to resolve

@rlespinasse

This comment has been minimized.

rlespinasse commented May 19, 2015

I check your code on https://github.com/yloiseau/golo-lang/blob/wip/macro-quote, and I am of the same opinion as you on the difficulty of resolution.

No need to wait your PR 😄

@jponge

This comment has been minimized.

Member

jponge commented May 19, 2015

@yloiseau

This comment has been minimized.

Contributor

yloiseau commented May 19, 2015

And indeed your refactoring is realy nice

@yloiseau

This comment has been minimized.

Contributor

yloiseau commented May 19, 2015

Moreover my PR is not ready yet

@jponge

This comment has been minimized.

Member

jponge commented May 21, 2015

All good to me.

jponge added a commit that referenced this pull request May 21, 2015

Merge pull request #266 from rlespinasse/wip/extend-cli
Ease the creation of others sub-commands for CLI

@jponge jponge merged commit 64e894f into eclipse:master May 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rlespinasse rlespinasse deleted the rlespinasse:wip/extend-cli branch May 21, 2015

@jponge

This comment has been minimized.

Member

jponge commented May 21, 2015

Just tried on a local copy and I have a bug :-)

$ target/golo-3.0.0-SNAPSHOT-distribution/golo-3.0.0-SNAPSHOT/bin/golo diagnose --tool ast oppriority.golo
Can't instantiate validator:java.lang.InstantiationException: fr.insalyon.citi.golo.cli.command.DiagnoseCommand$DiagnoseModeValidator

Diagnosis for the Golo compiler internals
Usage: diagnose [options] Golo source files (*.golo and directories)
  Options:
    --tool
       The diagnosis tool to use: {ast, ir}
       Default: ir

@rlespinasse rlespinasse restored the rlespinasse:wip/extend-cli branch May 21, 2015

@rlespinasse rlespinasse deleted the rlespinasse:wip/extend-cli branch May 21, 2015

@rlespinasse

This comment has been minimized.

rlespinasse commented May 21, 2015

it's link to a package protected class (a.k.a hidden for the Main class point of view)
https://github.com/golo-lang/golo-lang/blob/master/src/main/java/fr/insalyon/citi/golo/cli/command/DiagnoseCommand.java#L124

$ golo diagnose
(OK)
$ golo diagnose --tool ast ...
Can't instantiate validator:java.lang.InstantiationException: fr.insalyon.citi.golo.cli.command.DiagnoseCommand$DiagnoseModeValidator

Which do you prefer?

  • public static inner classes
  • public classes

Two commands in error : golo diagnose and golo doc

@jponge

This comment has been minimized.

Member

jponge commented May 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment