Make snakecase the default in Apply#119
Conversation
| name = fieldName | ||
| default: | ||
| unknown = true | ||
| // Use snakecase as the default. |
There was a problem hiding this comment.
Use fallthrough to avoid duplicating logic here.
Also, any observable behavior change like this should have a corresponding change in documentation.
madelinekalil
force-pushed
the
snakecase-default-apply
branch
2 times, most recently
from
f3147e1 to
f28c0a5
Compare
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
Madeline, there's something I'm not clear on. In the documentation for the gomodifytags command, it doesn't document "notransform" as an available option. Therefore, is there any need for the NoTransform constant at all? Can SnakeCase be the zero value? I think we should only make this change if SnakeCase can be the zero value.
| key = split[0] | ||
| name = strings.Join(split[1:], "") | ||
| } else if unknown { | ||
| // the user didn't pass any value but want to use an unknown |
There was a problem hiding this comment.
How would we have hit this code before? Looks like it wasn't exercised by any tests, since no tests had to change when this was removed. Would we have hit a parse error before getting here if the given transform was invalid?
There was a problem hiding this comment.
Thanks for the review! There were not any tests with invalid transform values, which is when we would reach this unknown case. I think it makes more sense to throw an error if the user passes an invalid transform value, and remove the NoTransform constant, so then SnakeCase can be the zero value. I added a test case for this error behavior in parseFlags.
madelinekalil
force-pushed
the
snakecase-default-apply
branch
from
f28c0a5 to
a18d651
Compare
| } | ||
|
|
||
| if cfg.fset == nil { | ||
| return errors.New("no file set") |
There was a problem hiding this comment.
This check isn't necessary here because we create a new fset for the file later in parse(), which is called after validate()
|
@madelinekalil let me know if you have anything else waiting, otherwise I'll merge it. |
3 participants
Currently, when calling Apply() without providing a Transform value, gomodifytags throws an error "unknown transform option". We should instead have a default value of snakecase, which is consistent with the command line execution of gomodifytags.