CLI history skips blank lines. #4768
Conversation
| // write out the history | ||
| if len(historyFile) > 0 { | ||
| select { | ||
| case <-c.Quit: |
There was a problem hiding this comment.
Does this mean I will only get history written if I type exit at the CLI?
There was a problem hiding this comment.
This structure doesn't really gain anything over a simple for loop since there are no goroutines running here. The agreement that ParseCommand offers is that it returns true if parsing should continue, false otherwise.
|
OK, I see that you state that history is only written if I enter |
|
@otoolep you are right and I've just reverted to previous behavior. I've updated the commit message and PR description. Anything else? |
|
I don't really see the point of the |
|
@otoolep the |
|
Also, one will be able to gracefully terminate the CLI from anywhere in the CLI code. |
|
OK, that is a valid point about The bigger issue here is that the CLI structure is not super. I'm not sure if you up for some more refactoring, but this code definitely needs it. Parsing and execution are coupled too tightly together. |
|
@otoolep my idea is to iteratively refactor the code. Big changes tend to be harder to review and merge. I do agree parsing should be split from execution. I have said before I want to integrate the parser in order to validate queries before executing them against a server, etc. But for this to happen, I'll need more time with the code and debate online with you guys. |
|
Sure -- I am happy to support you @pires. To that end, do you think the addition of |
| } | ||
| } | ||
|
|
||
| Loop: |
There was a problem hiding this comment.
Would love to see this go away in a future iteration. In general I don't like to use goto if we can avoid it.
There was a problem hiding this comment.
I actually found about labels in Go by looking at other pieces of the InfluxDB code. And while I never liked labels either, at this time it's a compromise I can live for the time being.
|
Thank you @otoolep. If there's a better alternative to explicitly trigger program termination from anywhere in this code other than |
|
@pires we really need the cli to support a signal exit like we do in influxd: https://github.com/pires/influxdb/blob/4719-cli_history_refactor/cmd/influxd/main.go#L90 Right now if we do a force quit it will leave your terminal in a messed up state as it doesn't cleanly shut down the |
|
@corylanou I agree I can improve the shutdown code and will do it in this PR. Will add support to OS signals as well. |
|
@corylanou done. Anything else? |
| fmt.Printf("There was an error writing history file: %s\n", err) | ||
| } | ||
| // exit CLI | ||
| os.Exit(0) |
There was a problem hiding this comment.
I think we really need a "close" function here. Then we can do something like:
if c.Line != nil {
c.Line.Close()
c.Line = nil
}And any other cleanup we need.
If we do an os.Exit without closing the liner down that is what will leave your terminal in a bad state.
|
@corylanou anything else? |
|
Very nice. Thanks for all your hard work on this. It's very much appreciated. +1 |
|
@corylanou @otoolep thank you for your guidance and patience. |
|
Is anything missing for this to be merged? |
|
@otoolep want to take one more look now that it's done? |
| if e != nil { | ||
| break | ||
| } | ||
| // write valid commands only to in-memory history |
|
All looks good to me, one question about a comment. +1 from me, @corylanou -- please merge when ready. Thanks for all the hard work @pires |
|
@otoolep @corylanou nit fixed. Thank you all, once more. |
CLI history skips blank lines.
Also:
cmd/influxlint.cmd/influxFixes #4719