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

Change println() to println(), disable event expansion #3

Merged
merged 2 commits into from
Mar 10, 2017

Conversation

C0urante
Copy link
Contributor

@C0urante C0urante commented Feb 9, 2017

Currently, queries like

SELECT * FROM orders WHERE orderunits != 5

will fail in CLI mode because JLine will attempt to expand the '!=' as an event. Disabling event expansion prevents this from happening and allows queries containing '!' to pass unchanged through the console reader to the parsing phase. The only downside I can think of is the loss of that shell-like functionality where you can use '!!' as a shortcut to represent the last command you entered and '!foo' to represent the last command you entered that started with 'foo'... probably not a huge deal, especially right now.

Also, small thing, but calls to println() with the empty string as an argument are redundant, as println() has a no-args version that does the exact same thing.

@C0urante
Copy link
Contributor Author

C0urante commented Feb 9, 2017

@hjafarpour LGTY?

@C0urante
Copy link
Contributor Author

@hjafarpour ping

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

LGTM. The loss of that behavior seems completely ok to me and better than having to remember to escape ! (which will probably be very common).

@hjafarpour Any thoughts?

@ewencp ewencp requested a review from hjafarpour March 9, 2017 23:40
@ewencp
Copy link
Contributor

ewencp commented Mar 9, 2017

Oh, and meant to say that cleanups like removing those empty strings are very welcome.

@norwood
Copy link
Contributor

norwood commented Mar 9, 2017

only thing i'd add is that it would be better to separate out the changes in to two commits. if for some reason we need to revert the functionality change, we wouldn't want to revert the style change with it

@ewencp
Copy link
Contributor

ewencp commented Mar 10, 2017

👍 great idea @norwood

@C0urante
Copy link
Contributor Author

@norwood reverted the commit and separated the two changes into different commits.

@norwood
Copy link
Contributor

norwood commented Mar 10, 2017

now for me to be a super pedant.

you should remove the original commit and the revert from the pr. they are states we shouldnt care about in master.

git rebase --interactive HEAD~5. mark b20adbc and f104554 as delete. then push

@C0urante
Copy link
Contributor Author

I ❤️ pedantry. Look good now?

@norwood
Copy link
Contributor

norwood commented Mar 10, 2017

its beautiful 😂 🎉

Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

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

LGTM!

@C0urante C0urante merged commit 9e201ef into master Mar 10, 2017
@C0urante C0urante deleted the cli-console-reader branch March 10, 2017 21:23
@miguno miguno mentioned this pull request May 3, 2018
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.

4 participants