Skip to content

Conversation

@deining
Copy link
Contributor

@deining deining commented Oct 26, 2020

This PR adds adds the Kotlin version for the Micronaut example (chapter 22).

When working on this, I extended the description a bit.
Also, the example was sligthly altered, it now closely resembles the auto-generated sample source file.

Also I removed these lines

@Client("https://api.github.com")
@Inject
var client: RxHttpClient? = null

since they weren't used inside the sample. As a side effect, number of imports could be reduced after removal of these lines.

Feel free to polish my description!

@remkop
Copy link
Owner

remkop commented Oct 26, 2020

I would prefer to keep the example injected service. They illustrate that services can be injected thanks to the factory that enables dependency injection. The business logic is omitted, so it is not clear how they are used, that is true.

Perhaps we can clarify a little by adding a comment?

    @Client("https://api.github.com")
    @Inject RxHttpClient client; // example injected service

...

    public void run() {
        // business logic here
        // injected services can be used here
    }

@codecov-io
Copy link

codecov-io commented Oct 27, 2020

Codecov Report

Merging #1232 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1232   +/-   ##
=========================================
  Coverage     94.33%   94.33%           
  Complexity      455      455           
=========================================
  Files             2        2           
  Lines          6686     6690    +4     
  Branches       1799     1799           
=========================================
+ Hits           6307     6311    +4     
  Misses          102      102           
  Partials        277      277           
Impacted Files Coverage Δ Complexity Δ
src/main/java/picocli/CommandLine.java 94.17% <100.00%> (+<0.01%) 314.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 fc5ef6d...1d42070. Read the comment docs.

@deining
Copy link
Contributor Author

deining commented Oct 27, 2020

I would prefer to keep the example injected service.

I do understand that, and in general I'm not against including these lines at all.

They illustrate that services can be injected
thanks to the factory that enables dependency injection.

That's true. Without having these lines use of Micronaut is overhead without real value.

My point is: without these lines, there is only single dependency for that sample: io.micronaut.configuration:micronaut-picocli. By adding these lines, you need a lot more dependencies which make the example more difficult to set up. And, based von my experience, improper setup of these dependiencies may cause the program to fail. And I simply want to save users from that potentially frustrating experience(s).

Proposal 1: we keep the lines as comments:

    // in a real life app, comment out the lines below
    // @Client("https://api.github.com")
    // @Inject RxHttpClient client; // example injected service

...

    public void run() {
        // business logic here
        // injected services can be used here
    }

Proposal 2: we show and explain the example from the micronaut quick start guide. Currently, the example is quite long, we might be able to shorten it a bit. This would make obvious to the user how the injected services are used.

Proposal 3: ?? (any comments or ideas for a completely different sample are welcome!).

Looking forward to your comment!

@remkop
Copy link
Owner

remkop commented Oct 27, 2020

How about something like this:

    @Client("https://api.github.com")
    @Inject RxHttpClient client; 

    @Parameters(description = "GitHub slug", paramLabel = "<owner/repo>") 
    String slug = "remkop/picocli";

    public void run() { 
        Map m = client.retrieve(GET("/repos/" + slug).header(
                "User-Agent", "remkop-picocli"), Map.class).blockingFirst();
        System.out.printf("%s has %s stars%n", slug, m.get("watchers"));
    }

Unsure if we need to add details about what dependencies are needed to run the example.

What do you think?

@deining
Copy link
Contributor Author

deining commented Oct 27, 2020

How about something like this:

    @Client("https://api.github.com")
    @Inject RxHttpClient client; 

    @Parameters(description = "GitHub slug", paramLabel = "<owner/repo>") 
    String slug = "remkop/picocli";

    public void run() { 
        Map m = client.retrieve(GET("/repos/" + slug).header(
                "User-Agent", "remkop-picocli"), Map.class).blockingFirst();
        System.out.printf("%s has %s stars%n", slug, m.get("watchers"));
    }

That's a great idea. I took that over into the PR 1:1.

Unsure if we need to add details about what dependencies are needed to run the example.

You can use the --feature xxxx option of the mn create-cli-app subcommand to specify dependencies. This is straightforward an worked for me. I added this to the PR.

What do you think?

I think, together we could improve the sample. Thanks for your input and thanks for reviewing my PR.

@remkop remkop added this to the 4.6 milestone Oct 27, 2020
@remkop remkop merged commit 1aefb99 into remkop:master Oct 27, 2020
@remkop
Copy link
Owner

remkop commented Oct 28, 2020

Merged, and published the rendered HTML.

Very nice. That section is certainly a lot more comprehensive now.
That reminds me that I should update the section for Quarkus: they have much better picocli support now than when I wrote that section.

remkop added a commit that referenced this pull request Oct 28, 2020
@deining deining deleted the micronaut-docu branch October 28, 2020 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants