-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Improve CLI command parsing #4704
Conversation
While this fixes the issue it can be further improved.
While I understand one may argue this tool is not meant to be dumb-proof, I like to make it harder to commit mistakes while using it. I'll probably be looking at fixing the above. |
This makes sense. Can you add some tests that show what was fixed? |
Sure. |
@corylanou care to review? |
@@ -216,4 +252,5 @@ func TestParseCommand_InsertInto(t *testing.T) { | |||
t.Fatalf(`Command "insert into" rp parsing failed, expected: %q, actual: %q`, test.rp, m.RetentionPolicy) | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like we picked up an extra line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
One nit, otherwise +1. Thanks again! |
@corylanou nit fixed. Thank you. |
This needs rebasing now. |
Makes sense, +1. |
Let us know when you rebase @pires and I will then merge. |
@otoolep ready. |
Thanks again @pires |
Now, the output:
Refs #4588
Fixes #4544