-
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean I will only get history written if I type exit
at the CLI?
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.
Yes.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Agreed.
@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 |
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.
Is this comment accurate?
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.
Nop. Thanks!
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/influx
lint.cmd/influx
Fixes #4719