Skip to content
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

Merged
merged 2 commits into from
Nov 10, 2015
Merged

Improve CLI command parsing #4704

merged 2 commits into from
Nov 10, 2015

Conversation

pires
Copy link
Contributor

@pires pires commented Nov 7, 2015

  • CHANGELOG.md updated
  • Rebased/mergeable
  • Tests pass
  • Sign CLA (if not already signed)

Now, the output:

$ influx -host 192.168.99.100
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://192.168.99.100:8086 version 0.9.3
InfluxDB shell 0.9
> user
ERR: error parsing query: found USER, expected SELECT, DELETE, SHOW, CREATE, DROP, GRANT, REVOKE, ALTER, SET at line 1, char 1
Warning: It is possible this error is due to not setting a database.
Please set a database with the command "use <database>".
> use
Could not parse database name from "use".

Refs #4588
Fixes #4544

@pires
Copy link
Contributor Author

pires commented Nov 7, 2015

While this fixes the issue it can be further improved.

>     use metrics
Using database metrics
> use  metrics
Could not parse database name from "use  metrics".

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.

@corylanou
Copy link
Contributor

This makes sense. Can you add some tests that show what was fixed?

@pires
Copy link
Contributor Author

pires commented Nov 7, 2015

Sure.

@pires
Copy link
Contributor Author

pires commented Nov 7, 2015

@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)
}
}

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@corylanou
Copy link
Contributor

One nit, otherwise +1. Thanks again!

@pires
Copy link
Contributor Author

pires commented Nov 9, 2015

@corylanou nit fixed. Thank you.

@otoolep
Copy link
Contributor

otoolep commented Nov 9, 2015

This needs rebasing now.

@otoolep
Copy link
Contributor

otoolep commented Nov 9, 2015

Makes sense, +1.

@otoolep
Copy link
Contributor

otoolep commented Nov 9, 2015

Let us know when you rebase @pires and I will then merge.

@pires
Copy link
Contributor Author

pires commented Nov 9, 2015

@otoolep ready.

@otoolep
Copy link
Contributor

otoolep commented Nov 10, 2015

Thanks again @pires

otoolep added a commit that referenced this pull request Nov 10, 2015
@otoolep otoolep merged commit baf83dd into influxdata:master Nov 10, 2015
@pires pires deleted the 4544-cli_smarter_parsing branch November 10, 2015 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants