Open woodensquares opened 6 years ago
Thank you very much for this pull request.
It might take me a couple of days to go through the code and be back with feedback, so stay tuned !
sure, thanks for the update!
@woodensquares Again, really really sorry for the delay, sometimes, life happens in the meantime 😊
I skimmed through the diff, and I have 2 (general) questions/remarks:
VarOpt
and VarArg
?Action
functions.@jawher no problem about the delay! From my perspective as much as you can achieve similar results with VarOpt
and VarArg
, enums as a first class type in the library seem more user-friendly also being integrated with the automatic help generation, making it easier to keep everything in sync, and decoupling the internal enum value from the user-visible one.
I think this in general is also introducing the concept of having the library do a little bit of validation, which is what I was getting at with the StringV
/ IntV
comment (where with some additional enhancements you could have the library, say, validate that user-supplied numbers are between 0 and 1024 or a string is a valid IPv4 address, etc. etc.). Again all things a user could do in their own code, but that would be nice if it could be offloaded to the library.
When it comes to the callbacks given that sometimes enums are used for fire-and-forget actions at the beginning of the execution, it seemed it would make user's life easier to allow them to optionally just write a one-liner callback (like the log level set in my example) rather than having to write more code afterwards to switch on the value. That would definitely be completely optional of course, the more typical behavior would of course be to return the user value, if you don't think this fits in the library as a workflow concept I am completely ok taking it out of course.
I wish this was merged back when it was fresh, I could really use this feature. Is it possible to achieve now the same goal (excluding automatic help generation) with cmd.Spec
?
Custom option/argument type let you implement your own enum type. Alos, since #96, help messages can be nicely formatted.
Here's an example: https://gist.github.com/jawher/8b3720e5a166e5219397ce2b8c8f7ddb
Here's how the help message would look like:
Usage: enum [OPTIONS]
enum usage example
Options:
-m file open mode
- read: read mode
- write: write mode (default )
-f, --force force open
And if you pass an invalid enum value, you get a clear error message:
./mowenum -m reqd
Error: invalid value "reqd". Must be one of: "read", "write"
Usage: enum [OPTIONS]
enum usage example
Options:
-m file open mode
- read: read mode
- write: write mode (default )
-f, --force force open
NOTE: This is just an initial pull request to allow for comments, it is not yet fully ready to be merged due to needing more unit test coverage (I just have a few basic tests for the time being), but I figured I'd open it now to make sure you are ok with this approach, if you are I will add the other needed tests in commands_test.go and possibly in internal/ (haven't fully investigated the tests there) to make sure the coverage does not decrease.
This is related to the discussion in issue #5. Note I have not squashed the pull intentionally, as hopefully that makes it easier to understand what I did.
Basically I really like mow.cli but also need enums and wanted them integrated in the help, so I thought I would take a stab at implementing them, I also added a way to have a callback in the enums, and made the validation hopefully extensible with an eye to supporting users creating StringV and IntV arguments where they could set things like "validate the string as an ipv4 address" or "this int must be between 0 and 1024"
You can kind of see this from the unit tests below, but anyways this is what it looks like in the code: a simple enum, user can set Red/Green/Blue and the value returned is r, g or b
An enum with callbacks, useful for logging for example
You can see the help in the golden file, but anyways it's something like this
Note the pull also has a change to the default value printing in the help, if you try the test case in master you should see the failure, and it seems related to assignments being done in one case but not in the other. If you can think of a cleaner way to fix this it'd be great, from my perspective I think that defaults can be calculated when the initial value is constructed so the approach seems fine, but of course let me know what you think.