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

Support for flag parameters in comments of a flag #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mikkelmr
Copy link

@mikkelmr mikkelmr commented Nov 7, 2019

This commit introduces the ability to add a parameter describtion for a flag, so it will appear in the in the syntax of the doc of the cli.
This way you can write clis that gives better hinting of how to use the cli based app.

Parameters are marked with '@param' following the Javadoc-styled documentation standard.
The following example has a flag called 'path' that expects a directory name

class MyFlags {
/**
@param
Search in 'directory'
**/
@:flag('path')
public var path:String = null;
.
.
.
}

Cli.getDoc(new MyFlags(), new tink.cli.doc.DefaultFormatter() will then give this output:

Usage:
--path, -p : Search in path of 'directory'

  • Only the first occurence of @param is used since a flag only accepts one value,
  • Lines with @params are removed from the comments before the are added to the doc of the flag.

This commit introduces the ability to add a parameter describtion for a flag, so it will appear in the in the syntax of the doc of the cli.
This way you can write clis that gives better hinting of how to use the cli based app.

Parameters are marked with '@param' following the Javadoc-styled documentation standard.
The following example has a flag called 'path' that expects a directory name

class MyFlags {
 /**
        @param <directory>
        Search in 'directory'
 **/
@:flag('path')
 public var path:String = null;
 .
 .
 .
}

Cli.getDoc(new MyFlags(), new tink.cli.doc.DefaultFormatter() will then give this output:

Usage:
 --path, -p<directory> : Search in path of 'directory'

- Only the first occurence of @param is used since a flag only accepts one value,
- Lines with @params are removed from the comments before the are added to the doc of the flag.
@kevinresol
Copy link
Member

I prefer this to be handled in a Formatter.
i.e. the macro should return the code comment as-is (and it has already done so)

@kevinresol
Copy link
Member

In principal I think this is a good addition. Just please move the parsing logic to runtime and leave the macro untouched 😉 .

@grepsuzette
Copy link

grepsuzette commented Nov 7, 2019

I've been using a custom Formatter for basic similar reason. So it's probably a good idea to improve it.

However I have a problem with the name @param. I find it really shocking and surprising in this context, because this is not the help for the code but for the user, whereas @param is normally for embedded documentation to document the code.

Instead I propose something like @tip ..., if such a line exists in the block.

@help ... probably works even better. It's obvious and easy to remember and probably will never be used in an embedded doc system.

@kevinresol
Copy link
Member

Yeah the default formatter was quickly thrown together so I could have something to display. I am more than happy to see PRs that put in some advanced formatters.

@mikkelmr
Copy link
Author

mikkelmr commented Nov 7, 2019

@emugel my initial attempt was actually a solution where I had a @:tip meta decoration on the flag field, and on second thoughts I maybe like that approach better. Like this:

@:tip('')
public var path: String

@:tip('<var[=value]>')
public var defines: Array

That way we could avoid polluting the comments section, with stuff that's not meant to be shown in the doc. Using the comments meta data for the docFormatter is a creative(but clever) way of the exploiting the Haxe macro system anyway.

@kevinresol do you mean that I should not make any changes in the Macro.hx file?

@kevinresol
Copy link
Member

Yes please

@grepsuzette
Copy link

@:tip("Activate verbose output")
public var verbose : Bool = false;

vs

@:doc("Activate verbose output")
public var verbose : Bool = false;

vs

@:help("Activate verbose output")
public var verbose : Bool = false;

vs

@:cli("Activate verbose output")
public var verbose : Bool = false;

I think doc < tip < help < cli. What's your preferences?

@mikkelmr
Copy link
Author

mikkelmr commented Nov 8, 2019

@emugel are you suggesting to move the doc contents from comments section to a metadata attribute?

If so it could be like this, and imo that would be the cleanest and most intuitive solution:

    /**
    Normal comments that will be ignored by the cli
   **/
    @:flag('path')
    @:tip('<directory>')
    @:doc('
    Documentation for flag path.
    Newlines need to works as well')
    public var path : String = "";

Disadvantage would be that it would not be backwards compatible since implementations that relies on cli documentation in the normal comments would have to move it to the @:doc metadata.

@kevinresol
Copy link
Member

I don't see the point of introducing new meta just to hold the documentation. The Haxe language itself already support that through field docs. You can put any strings in it and parse it at runtime.

@mikkelmr
Copy link
Author

mikkelmr commented Nov 8, 2019

@kevinresol do you mean that we use the docs to place everything relevant for the doc formatter and then have the formatter parse it and strip stuff out that should not be shown?

@grepsuzette
Copy link

grepsuzette commented Nov 8, 2019

@mikkelmr doc < tip < help < cli was an attempt to sort my preferences from least to most, with cli my favourite. But I am not sure anymore and Kevin will know better anyway since he made this lib.

Until now I document my fields like that:

/** Enable verbose output */
public var verbose : Bool;   // code documentation invisible to the user goes here
                                         // continues below if too long

Proposition 1 is to have it this way, whatever the name I just use @cli as an example here:

/**
 * @cli Enable verbose output
 * code documentation invisible to the user goes here
 * and can be very long
 */
@:alias("verbose") @:flag("v") public var isVerbose : Bool;

Proposition 2 is to have it like:

/** code documentation invisible to the user goes here 
 * and can be very long */
@:cli("Enable verbose output")
@:flag("verbose") @:alias("v") public var isVerbose : Bool;

Provided the old way still works, I think I slightly prefer proposition 1, as in your original PR. It's easier on the eyes, and the programmer may start using /** user-doc goes here */ and add a @whatever on this one line if he wishes to use the rest of the documentation block for code documentation.

@kevinresol
Copy link
Member

@kevinresol do you mean that we use the docs to place everything relevant for the doc formatter and then have the formatter parse it and strip stuff out that should not be shown?

correct

@mikkelmr
Copy link
Author

mikkelmr commented Nov 10, 2019

I have now removed the changes to the macro as discussed. However the implementation is still using "@param" and I'm not sure weather we settled on that @emugel and @kevinresol? There was also a suggestion to use "@cli" to only have the part that should be shown to the user, thus making the comments section work as usual.

@@ -71,20 +85,27 @@ class DefaultFormatter implements DocFormatter<String> {

var maxFlagLength = spec.flags.fold(function(flag, max) {
var name = nameOf(flag);
name += ' ${getParamDesc(flag)}';
Copy link
Member

Choose a reason for hiding this comment

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

Seems the quotes are unnecessary?

@kevinresol
Copy link
Member

@mikkelmr Thanks for the update. But can you leave the Macro.hx file unchanged please?

@grepsuzette
Copy link

@mikkelmr I’m a mere user of tink_cli so the final decision about using @param or another variant does not belong to me :)

@kevinresol
Copy link
Member

@emugel My general principal is not to introduce any new metadata for documentations. Other than that I don't have much opinion. Because the default formatter was just something that I quickly thrown together. It doesn't really have any "standard" or "design" so basically I will be fine with any changes to it.

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.

3 participants