Closed RiyaJohn closed 4 years ago
@dnnrly Please review this PR
First of all, I want to say a massive thank you for putting this PR forward!
I've been looking at it and it looks like it solves the problem exactly as I specified in the issue. Unfortunately, it's made it obvious just how badly I messed up the description and asked for the wrong thing.
So my concern here is that we're only solving single specific cases but we have an opportunity to fix it generally, and better. I like that you renamed the function, and perhaps we can start there. What if it were renamed (again) to AsSeparated
? Then we can keep the snake
and kebab
commands and add a new separated
command that requires a separator option.
I think the upshot of this is that we could remove the extra unit tests that you added to support kebab case. I think we can get rid of the additional test cases you added, I think these are already covered by the existing ones - I think I kind of half-arsed the implementation without thinking too much how the CLI would look.
Once all that is done (it's not a massive job) we can add a couple of test cases to acceptance.bats
to prove that we got the plumbing right.
How does that sound?
I think when all this is done, you've earned yourself a new follower on Twitter. 😄
First of all, I want to say a massive thank you for putting this PR forward!
I've been looking at it and it looks like it solves the problem exactly as I specified in the issue. Unfortunately, it's made it obvious just how badly I messed up the description and asked for the wrong thing.
So my concern here is that we're only solving single specific cases but we have an opportunity to fix it generally, and better. I like that you renamed the function, and perhaps we can start there. What if it were renamed (again) to
AsSeparated
? Then we can keep thesnake
andkebab
commands and add a newseparated
command that requires a separator option.I think the upshot of this is that we could remove the extra unit tests that you added to support kebab case. I think we can get rid of the additional test cases you added, I think these are already covered by the existing ones - I think I kind of half-arsed the implementation without thinking too much how the CLI would look.
Once all that is done (it's not a massive job) we can add a couple of test cases to
acceptance.bats
to prove that we got the plumbing right.How does that sound?
I think when all this is done, you've earned yourself a new follower on Twitter. 😄
This sounds good, I like it this way. I will make the changes
@dnnrly I made the changes, removed option for separator flag for snake and kebab commands. But let me know if we wanna retain it
Description
Adding support for kebab case
Fixes #16
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
make lint
)master