Skip to content
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

Docu, new code samples: Kotlin versions, minor changes #1296

Merged
merged 5 commits into from
Jan 5, 2021
Merged

Docu, new code samples: Kotlin versions, minor changes #1296

merged 5 commits into from
Jan 5, 2021

Conversation

deining
Copy link
Contributor

@deining deining commented Jan 4, 2021

This PR adds Kotlin versions for the new code samples in picocli 4.6 (IModelTransformer, IParameterProcessor).
While working on the IParameterProcessor sample, I made minor changes to the code sample and to the documentation in order to improve readability and intelligibility. I hope you like them.
Additionally, I added a third column to the comparison table PicocliBaseScript2 vs. PicocliBaseScript.

@codecov-io
Copy link

codecov-io commented Jan 4, 2021

Codecov Report

Merging #1296 (c0bd988) into master (97c73b1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1296   +/-   ##
=========================================
  Coverage     93.78%   93.78%           
  Complexity      469      469           
=========================================
  Files             2        2           
  Lines          6948     6948           
  Branches       1864     1864           
=========================================
  Hits           6516     6516           
  Misses          146      146           
  Partials        286      286           

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 97c73b1...c0bd988. Read the comment docs.

Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Wow, awesome work again!

The only think I would like to change is that I think we should keep Editor.defaultEditor to represent the absence of choice by the end user, rather than just picking one of the actual editors as the default. I put more detailed comments with the proposed changes.

Other than that, it looks great!

docs/index.adoc Outdated Show resolved Hide resolved
docs/index.adoc Outdated
"Example: edit --open=idea FILE opens IntelliJ IDEA (notice the '=' separator)",
" edit --open FILE opens the specified file in the default editor"
" edit --open FILE opens the specified file in eclipse"
})
Copy link
Owner

Choose a reason for hiding this comment

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

Similar to the above, I think it is meaningful to have a defaultEditor choice, so let's keep the original description.

docs/index.adoc Outdated Show resolved Hide resolved
docs/index.adoc Outdated
// act as if the user specified --open=defaultEditor
args.push(Editor.defaultEditor.name());
// act as if the user specified --open=eclipse
args.push(editor.name());
Copy link
Owner

Choose a reason for hiding this comment

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

I think it is good to explicitly use the enum constant that represents the default, so Editor.defaultEditor (or Editor.eclipse if Eclipse were the default).

This is mostly for clarity: so that people reading this code example in the documentation would not have to refer back in the code to what the actual value would be.

I realize that in "real" code there is a tension here with the principle of having information in only once place. To be honest, I am not sure what I would do in a "real" application in this case. :-)

But for documentation purposes, I believe that duplicating the default value in the preprocessor is better because it requires less work on the part of the reader.

docs/index.adoc Outdated Show resolved Hide resolved
@deining
Copy link
Contributor Author

deining commented Jan 5, 2021

Wow, awesome work again!

Great you like it!
I realized that the comparison table PicocliBaseScript2 vs. PicocliBaseScript is present in the release notes, too. For the sake of consistency, I would like to modify this table, too. I hope this is fine for you.

The only think I would like to change is that I think we should keep Editor.defaultEditor to represent the absence of choice by the end user, rather than just picking one of the actual editors as the default. I put more detailed comments with the proposed changes.

I will work on this shortly, you may expect an updated PR soon.

Other than that, it looks great!

Thank you!

While working on this, two further issues came up that I would like to raise:

  • I'm wondering, whether the API Docs for the package picocli.groovy are up to date. I would suspect these docs at this URL, but these documents are 4.0.0-alpha-3. I didn't find a link to these API docs in any README.mdfile in the repository either, I think this is something that should be added.
  • ParameterConsumer vs IParameterPreprocessor: AFAICS, the IParameterPreprocessor is superior to the ParameterConsumer in all aspects. Is there any reason to still stick to the ParameterConsumer? If not, this interface should be marked as deprecated IMHO. Also, chapter 11.15. Custom Parameter Processing lists both plugin in chronological order concerning their authoring date. I think they should be ordered according to relevance for the user, therefore I would suggest to swap the first two subchapters.

@remkop
Copy link
Owner

remkop commented Jan 5, 2021

I realized that the comparison table PicocliBaseScript2 vs. PicocliBaseScript is present in the release notes, too. For the sake of consistency, I would like to modify this table, too. I hope this is fine for you.

I'm okay to leave the release notes as they are, and just evolve the user manual. I see the release notes as more of a snapshot of how things were at some point in time, so there is some value in not updating them.

  • I'm wondering, whether the API Docs for the package picocli.groovy are up to date. I would suspect these docs at this URL, but these documents are 4.0.0-alpha-3. I didn't find a link to these API docs in any README.mdfile in the repository either, I think this is something that should be added.

Oh, good catch! I had completely forgotten about this...
The Groovy classes were previously in the main picocli jar, now they are in a separate jar... I suspect these javadoc pages are still there from when these classes were in the picocli jar.

It makes sense to publish Javadoc for the other jars, not just for the picocli jar, but also for picocli-groovy, picocli-jline2/3, picocli-spring-boot and picocli-codegen. The best way to deal with this is probably to update the copyDocs task in build.gradle. Also, like you said, we need to link to their top-level pages from the user manual and project README.

  • IParameterConsumer vs IParameterPreprocessor: AFAICS, the IParameterPreprocessor is superior to the IParameterConsumer in all aspects. Is there any reason to still stick to the IParameterConsumer? If not, this interface should be marked as deprecated IMHO. Also, chapter 11.15. Custom Parameter Processing lists both plugin in chronological order concerning their authoring date. I think they should be ordered according to relevance for the user, therefore I would suggest to swap the first two subchapters.

Swapping these in the manual makes sense; IParameterPreprocessor is more flexible than IParameterConsumer. That said, IParameterConsumer is simpler, so I can imagine people preferring to use that one. I am not in a rush to deprecate IParameterConsumer. It is probably a good idea to update the javadoc for IParameterConsumer to mention that IParameterPreprocessor is a more powerful and flexible alternative.

@deining
Copy link
Contributor Author

deining commented Jan 5, 2021

we need to link to their top-level pages from the user manual and project README.

As an addition, we could develop/author an overview page that contains links to the apidocs of all subprojects. Once we made this overview page avaible at the subdomain apidocs.picocli.info, this URL could serve as single starting point for accessing all apidocs (just a proposal/ an idea).

@remkop
Copy link
Owner

remkop commented Jan 5, 2021

I created #1298 and put some ideas on how to deal with the Javadoc for the other modules there, let's discuss further on that ticket.

@deining deining requested a review from remkop January 5, 2021 10:52
@deining
Copy link
Contributor Author

deining commented Jan 5, 2021

I resolved all issues from your review meanwhile.
Thanks for considering my PR.

@remkop remkop merged commit 286b756 into remkop:master Jan 5, 2021
@deining deining deleted the kotlin_samples branch January 5, 2021 12:10
@remkop remkop added this to the 4.7 milestone Jan 5, 2021
@remkop
Copy link
Owner

remkop commented Jan 5, 2021

Merged and published the HTML. Many thanks!

@remkop remkop modified the milestones: 4.7, 4.6.2 Feb 23, 2021
MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
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.

3 participants