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

Recreate the README #5

Merged
merged 40 commits into from
Nov 14, 2020
Merged

Recreate the README #5

merged 40 commits into from
Nov 14, 2020

Conversation

BradleyChatha
Copy link
Owner

See #4

@BradleyChatha BradleyChatha added the documentation Improvements or additions to documentation label Nov 3, 2020
Copy link
Contributor

@andrey-zherikov andrey-zherikov left a comment

Choose a reason for hiding this comment

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

Confusing terminology: "command" and "named/sub-commands" - please one term everywhere.

PS Not finished yet - will continue tomorrow

README.md Outdated
![Tests](https://github.com/SealabJaster/jcli/workflows/Test%20LDC%20x64/badge.svg)
![Examples](https://github.com/SealabJaster/jcli/workflows/Test%20Examples/badge.svg)
![Tests](https://github.com/BradleyChatha/jcli/workflows/Test%20LDC%20x64/badge.svg)
![Examples](https://github.com/BradleyChatha/jcli/workflows/Test%20Examples/badge.svg)

JCLI is a library to aid in the creation of command line tooling, with an aim of being easy to use, while also allowing
the individual parts of the library to be used on their own, allowing more dedicated users some tools to create their own
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the individual parts of the library to be used on their own, allowing more dedicated users some tools to create their own
the individual parts of the library to be used on their own, allowing more dedicated users to create their own

README.md Show resolved Hide resolved
README.md Outdated

JCLI has support for both a default command, and multiple sub-commands.
* Tested on Windows, and Ubuntu 18.04.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a feature

README.md Outdated

*Note: JCLI can technically support any shell's autocomplete, but only Bash is supported for now*.
## Creating your project
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this section as "creating project" is not what's provided by this package.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yea, I'm way too used to talking to people who are still new to coding/really bad at following directions, so I tend to over-explain and go off topic a bit.

README.md Outdated

I'm still a pretty big noob to Linux, and this feature is still in its early stages, so forgive me if things aren't exactly perfect here :o)
The default command is the command that is ran when you don't specify the name of a named command. e.g. `mytool 60 20 --some=args` would call the default command if it exists.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The default command is the command that is ran when you don't specify the name of a named command. e.g. `mytool 60 20 --some=args` would call the default command if it exists.
The default command is the command that is ran when you don't specify the name of a named command. e.g. `mytool 60 20 --some=args` would call the default command if it exists:

README.md Outdated

## Examples
To start off with, we need to import jcli and create a minimal amount of code for our command to exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To start off with, we need to import jcli and create a minimal amount of code for our command to exist.

README.md Outdated Show resolved Hide resolved
README.md Outdated

* [JCLI's manual/misc test program](https://github.com/SealabJaster/jcli_testerr)
All commands must define an `onExecute` function, which either returns `void`, or an `int` which will be used as the program's exit/status code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
All commands must define an `onExecute` function, which either returns `void`, or an `int` which will be used as the program's exit/status code.
All commands must define an `onExecute` function, which either returns `void`, or an `int` that will be used as the program's exit/status code.

README.md Outdated

## Changelog
Commands can either be a struct or a class, but for now we'll use structs as they're simpler.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be removed to not confuse users

Copy link
Owner Author

Choose a reason for hiding this comment

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

The feature itself, or just that part of the documentation?

I've moved it to somewhere a bit more appropriate in my latest changes either way.

Copy link
Contributor

@andrey-zherikov andrey-zherikov left a comment

Choose a reason for hiding this comment

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

  1. Why do you want users to do import jaster.cli , not something like import jcli?
  2. Please rename mytool to something meaningful in few first examples, like check_number. Also please change The default command to something meaningful.

PS still reading through...

README.md Outdated
12. [Dependency Injection](#dependency-injection)
13. [Calling a command from another command](#calling-a-command-from-another-command)
14. [Configuration](#configuration)
15. [Inheritance](#inheritance)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid manual numbering by putting 1. on every line. See https://gist.github.com/andrey-zherikov/b7f97f68e7ab8c2317162300192e2e9e (click on "raw" button)

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

* Bash Completion - JCLI provides automatic support for bash tab completion, see the appropriate section below for more information.
* Pass through unparsed arguments. (`./mytool command -- these are unparsed args`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Pass through unparsed arguments. (`./mytool command -- these are unparsed args`)
* Pass through unparsed arguments (`./mytool parsed args -- these are unparsed args`)

README.md Outdated

## Examples
* Uncategorised:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Uncategorised:
* Customizable design:

README.md Outdated

# Bad value for mode
$> mytool 60 --mode lol
std.conv.ConvException@\src\phobos\std\conv.d(2817): Mode does not have a member named 'reverse'
Copy link
Contributor

Choose a reason for hiding this comment

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

First of all, error text mentions reverse but lol is passed.

Error text should be better:

Suggested change
std.conv.ConvException@\src\phobos\std\conv.d(2817): Mode does not have a member named 'reverse'
Invalid value for argument '--mode': 'lol'

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's mostly down to laziness. Opened #10 for it though.

README.md Outdated
Comment on lines 330 to 336
Optional arguments, as the name implies, are optional. Only Named arguments can be optional (technically, Positional arguments can be optional in certain use cases, but JCLI doesn't support that... yet).

Inside of D's standard library - Phobos - is the module [std.typecons](https://dlang.org/phobos/std_typecons.html) which contains a type called [Nullable](https://dlang.org/phobos/std_typecons.html#Nullable).

JCLI has special support for this type, as it is used to mark an argument as optional. This type is publicly imported by JCLI, so you don't have to import anything extra.

Anyway, all we want to do is make our `mode` argument a `Nullable`, so JCLI knows it's optional:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional arguments, as the name implies, are optional. Only Named arguments can be optional (technically, Positional arguments can be optional in certain use cases, but JCLI doesn't support that... yet).
Inside of D's standard library - Phobos - is the module [std.typecons](https://dlang.org/phobos/std_typecons.html) which contains a type called [Nullable](https://dlang.org/phobos/std_typecons.html#Nullable).
JCLI has special support for this type, as it is used to mark an argument as optional. This type is publicly imported by JCLI, so you don't have to import anything extra.
Anyway, all we want to do is make our `mode` argument a `Nullable`, so JCLI knows it's optional:
JCLI supports optional arguments through standard [Nullable](https://dlang.org/phobos/std_typecons.html#Nullable) type. Note that only Named arguments can be optional for now (technically, Positional arguments can be optional in certain use cases, but it's not supported ... yet).
So to make our `mode` argument optional, we need to make it `Nullable`:

Technically "optional" means that it can be omitted in command line and so inside a program it might be either "not set" (e.g. null) or to be set to some default value so consider this use case:

    @CommandNamedArg("mode", "Which mode to use.")
    @DefaultValue("normal")
    Mode mode;

or:

enum {
    @CommandArgValue("normal", "Even returns 1. Odd returns 0.")
    @DefaultValue
    normal_mode,
    @CommandArgValue("reversed", "Even returns 0. Odd returns 1.")
    reverse_mode,
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

Unless I'm missing something, Nullable supports this does it not?

Nullable.isNull can be used to check if the user passed in a value or not.

Nullable.get as explained can be to get a default value if the user didn't pass a value in themselves.

README.md Outdated

Please see [this](https://github.com/SealabJaster/jcli/tree/master/examples/05-dependency-injection) example for more information.
The first change is that `Mode mode;` has now become `Nullable!Mode mode`, to make it, quite literally, a `Mode` that is `Nullable`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion (instead of few lines here) to remove some basic things:

There are two changes:

  • Type of mode member becomes Nullable!Mode.
  • onExecute now uses mode.get(Mode.normal) which returns Mode.normal is --mode option is not provided.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It was mostly to point out something that might be easy to glance over. I've removed the first line as its redundant with your above suggestion, but I've kept the second line.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andrey-zherikov andrey-zherikov left a comment

Choose a reason for hiding this comment

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

I'd separate these as "Basic usage":

    1. [Creating a default command](#creating-a-default-command)
    2. [Positional arguments](#positional-arguments)
    3. [Registering commands](#registering-commands)
    4. [Compiling and running your program (and help text)](#compiling-and-running-our-program-also-show-help-text)
    5. [Named arguments](#named-arguments)
    6. [Optional arguments](#optional-arguments)
    7. [Arguments with multiple names](#arguments-with-multiple-names)
    8. [Named commands/subcommands](#named-commands)
    11. [Unparsed Raw Arg List](#unparsed-raw-arg-list)

And these as "Advanced usage" (the rest):

    9. [User Defined argument binding](#user-defined-argument-binding)
    10. [User Defined argument validation](#user-defined-argument-validation)
    12. [Dependency Injection](#dependency-injection)
    13. [Calling a command from another command](#calling-a-command-from-another-command)
    14. [Configuration](#configuration)
    15. [Inheritance](#inheritance)```

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andrey-zherikov andrey-zherikov left a comment

Choose a reason for hiding this comment

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

In general I didn't get what dependency injection provides here.

Also the following info is missed (may be issues should be created for some?):

  • Case sensitivity: are options case sensitive or insensitive?
  • What value format is supported: --foo=value, --foo value or both?
  • How are boolean flags processed? Will --foo=true or --foo false work? Any support of std.typecons.Flag?
  • What happened if the same option is provided multiple times? For example, -v option for verbosity level might be used multiple times to increase verbosity: -v -v -v -v or -vvvv.
  • What happens if argument has array type (or associative array)? See std.getopt as an example of how this could be done.
  • How can I specify allowed values for an argument? For example, for type int I want to allow 2, 4 and 6 values only.
  • How can I group arguments the way I want? See here for example.
  • How can I specify mutual exclusion between arguments? There are also use cases with more complex dependencies between arguments (their appearance in command line, not their values).

PS I'm done with reading

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated

This is the function that performs the actual validation (in this case, "Pre" validation).

It returns `true` if there are no validation errors, otherwise it returns `false` and optionally sets the `error` string to a user-friendly error (one is automatically generated otherwise).
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add example of automatically generated error.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 683 to 692
So far whenever we've been testing our program, I've told you to do `dub build` into a `./mytools ...` command.

What if I told you we can just use a single dub command to do both at the same time?

`dub run` will both build and run the program, while also allowing us to pass arguments to our own program.

For example, instead of `./mytool cat dub.json` we can just do `dub run -- cat dub.json`, all the args after the double dash are passed
unmodified to our own program. JCLI refers to this as the "raw arg list".

So naturally, JCLI provides this feature as well using the exact same syntax:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make it shorter:

Suggested change
So far whenever we've been testing our program, I've told you to do `dub build` into a `./mytools ...` command.
What if I told you we can just use a single dub command to do both at the same time?
`dub run` will both build and run the program, while also allowing us to pass arguments to our own program.
For example, instead of `./mytool cat dub.json` we can just do `dub run -- cat dub.json`, all the args after the double dash are passed
unmodified to our own program. JCLI refers to this as the "raw arg list".
So naturally, JCLI provides this feature as well using the exact same syntax:
In some cases you might want to stop parsing arguments and just get them as raw strings. JCLI supports this use case by allowing raw arguments to appear after long double-dash (`--`) parameter in command line: `mytool args to parse -- args to pass as is`.
This feature can be used with this syntax:

README.md Show resolved Hide resolved
@BradleyChatha
Copy link
Owner Author

Right, so onto some of your other points I couldn't reply to directly.

Why do you want users to do import jaster.cli , not something like import jcli?

This is actually a really silly reason. I'm not terribly great with naming things, so when I make a new project I literally just take what it's about (in this case, a 'cli' library) and slap a 'j' (for Jaster, which'd make more sense if my Github name was still "SealabJaster") on the front of it.

And so, because I was planning on having several projects with that naming scheme, I kind of decided to name their modules things like jaster.cli, jaster.ioc, etc.

It's not terribly worth changing it I feel, but yeah there isn't really any appropriate reason for it >:)

Please rename mytool to something meaningful in few first examples, like check_number. Also please change The default command to something meaningful.

I kind of wanted something simple and generic to use throughout the entire README, so I think it's ok as it is. The default command is a bit basic, but that's mostly because we're teaching the user how to use JCLI, not write command logic.

In general I didn't get what dependency injection provides here.

Honestly it's only there for when its needed.

There wasn't any way to pass objects into a command's constructor, which could be useful for reasons such as taking the config loading logic outside of the command itself (multiple commands might use the same config, so it'd just be duplicate code). This is especially true since I wanted commands to only be created on command in an as-needed basis, so I wasn't sure what the best way forward was (e.g. you can't pass in a pre-constructed instance of a command. Had to define somehow what values to actually pass into its ctor, etc.).

So on a whim I kind of just decided "I'm going to make an IOC container and add dependency injection", since I wanted to have a library that did that anyway.

Again, it wasn't terribly well thought out, but it solves the small problem area in a ok-ish way for when the need arises. There's value in having it, even if not by much.

Case sensitivity: are options case sensitive or insensitive?

#14

What value format is supported: --foo=value, --foo value or both?

This information exists inside of ArgPullParser's documentation comment.

I 100% agree that it needs to be more visible though. Perhaps a separate section in the README?

How are boolean flags processed? Will --foo=true or --foo false work? Any support of std.typecons.Flag?

Defined in CommandLineInterface's documentation comment, which like above should be made more visible.

What happened if the same option is provided multiple times? For example, -v option for verbosity level might be used multiple times to increase verbosity: -v -v -v -v or -vvvv.

I don't believe this is ever stated anywhere, and honestly I completely (somehow) forgot about this case.

Current behaviour is that it just uses the last-found value (-a 1 -a 2 would only use -a 2) which may or may not be desirable. I feel that should become configurable, so I've made #15 for this.

Now, for your -v case I'd like to implement a feature that I've seen in phobos' getopt, which is to increment a counter every time the argument appears. I've gone ahead and create #16 for this. The latter syntax could technically be supported in this specific case as well, even though ArgPullParser doesn't directly support it.

What happens if argument has array type (or associative array)? See std.getopt as an example of how this could be done.

I've thought about this before, and I kind of just brushed it off as a "I'll get to it eventually" sort of thing.

Unless there's any specific use cases you can think of to support, I think arrays should function the same as in getopt.

#17

How can I specify allowed values for an argument? For example, for type int I want to allow 2, 4 and 6 values only.

For now, you'd have to use validators and manually state something in your argument's description about it:

@CommandNamedArg("number", "A number that can only be 2, 4, or 6.")
@OneOf([2, 4, 6]) // Custom validator
int number;

I'm open to suggestions on what improvements could be made though.

Talking about validators, I'd like to get a good list of ready-made validators made, so I'll be tracking that in #19

How can I group arguments the way I want? See here for example.

Ooh, that'd be very nice to have. Currently there is absolutely 0 support for that, but #20 will make sure that we have it.

How can I specify mutual exclusion between arguments? There are also use cases with more complex dependencies between arguments (their appearance in command line, not their values).

No support right now, but I'd like it (and coincidentally, I had a glancing thought about it today already).

The simple use case would also be incredibly simple to implement.

Your more complex use case would likely be doable as well, but I probably won't look into it straight away.

My biggest concern here is what should the user-facing code look like... #21

I hope I didn't miss anything, in either my replies or my latest changes.

And again, thank you for your time. I'm pretty happy someone's taken an interest in this library.

Copy link
Contributor

@andrey-zherikov andrey-zherikov left a comment

Choose a reason for hiding this comment

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

It's not terribly worth changing it I feel, but yeah there isn't really any appropriate reason for it >:)

You can provide jcli module that publicly imports jaster.cli.

I kind of wanted something simple and generic to use throughout the entire README, so I think it's ok as it is. The default command is a bit basic, but that's mostly because we're teaching the user how to use JCLI, not write command logic.

IMO it looks much better and clearer if example is meaningful and complete. So if you want to describe how to use the library by writing small tool then the tool should look like complete tool without dummy text.

In general I didn't get what dependency injection provides here.

Honestly it's only there for when its needed.

There wasn't any way to pass objects into a command's constructor, which could be useful for reasons such as taking the config loading logic outside of the command itself (multiple commands might use the same config, so it'd just be duplicate code). This is especially true since I wanted commands to only be created on command in an as-needed basis, so I wasn't sure what the best way forward was (e.g. you can't pass in a pre-constructed instance of a command. Had to define somehow what values to actually pass into its ctor, etc.).

Sorry, I still didn't get it (may be I need to dive deep into that?). If you are trying to implement the workflow where actual arg values are coming from different sources (like command line, config file etc.) then you can define source interface and separate argument parsing (i.e. validating, converting etc.) from reading input data.

What value format is supported: --foo=value, --foo value or both?

This information exists inside of ArgPullParser's documentation comment.

I 100% agree that it needs to be more visible though. Perhaps a separate section in the README?

Just add this to feature list.

How can I specify allowed values for an argument? For example, for type int I want to allow 2, 4 and 6 values only.

For now, you'd have to use validators and manually state something in your argument's description about it:

@CommandNamedArg("number", "A number that can only be 2, 4, or 6.")
@OneOf([2, 4, 6]) // Custom validator
int number;

I'm open to suggestions on what improvements could be made though.

I'd suggest the same approach I suggested for enums:

@CommandNamedArg("number", "A number that can only be 2, 4, or 6.")
@CommandArgValue(2, "optional description")
@CommandArgValue(4, "optional description")
@CommandArgValue(6, "optional description")
int number;

And again, thank you for your time. I'm pretty happy someone's taken an interest in this library.

np. Actually there are not so many libs to choose from (basically jcli, cli-d and std.getopt) and jcli matches better my use case (subcommands) although it has lack of functionality (that's why we are discussing everything here). And my biggest concern is that jcli pulls in other dependencies (jioc) which IMO shouldn't be done by argument parsing library.

README.md Outdated Show resolved Hide resolved
@Command(null, "The default command.")
struct DefaultCommand
{
@CommandPositionalArg(0, "number", "The number to check.")
Copy link
Contributor

Choose a reason for hiding this comment

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

The other reasoning is that every time when positional argument is removed or inserted, indexes have to be updated.
Technically I see no problem in providing both options - just need two ctors for CommandPositionalArg: one with index, another without.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@BradleyChatha
Copy link
Owner Author

Been feeling a depressive spiral coming on, so I've rushed through all the PRs so I can make a release, then try relax for a week or so.

@BradleyChatha BradleyChatha merged commit be1174e into master Nov 14, 2020
@BradleyChatha BradleyChatha deleted the new-readmes branch May 9, 2021 03:23
@BradleyChatha BradleyChatha restored the new-readmes branch May 9, 2021 03:23
@BradleyChatha BradleyChatha deleted the new-readmes branch May 9, 2021 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants