-
Notifications
You must be signed in to change notification settings - Fork 57
A possible way to add enums #63
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
base: master
Are you sure you want to change the base?
Conversation
…alidate the default values.
…g on when printvalues is called
|
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:
|
|
@jawher no problem about the delay! From my perspective as much as you can achieve similar results with 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 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 |
|
Custom option/argument type let you implement your own enum type. Here's an example: https://gist.github.com/jawher/8b3720e5a166e5219397ce2b8c8f7ddb Here's how the help message would look like: And if you pass an invalid enum value, you get a clear error message: |
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.