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

implemented DELIMITER command #782

Merged
merged 2 commits into from
Oct 13, 2019
Merged

implemented DELIMITER command #782

merged 2 commits into from
Oct 13, 2019

Conversation

gfrlv
Copy link
Member

@gfrlv gfrlv commented Oct 5, 2019

Description

#383
DELIMITER command to switch the default delimiter ; to something else. Copied the behavior from the standard mysql shell.

Splitting lines is based on a suggestion by @srittau as a temporary workaround until custom delimiters get into the sqlparse library

One limitation is that delimiter cannot be used in a favorite query, for example if you save

\fs foo delimiter $ select 1 $ select 2 delimiter ;

then \f foo will result in a syntax error, although a user may actually want to save such a query, e.g. to do some work on stored procedures. I can imagine refactoring favorite queries so that they are executed with SQLExecute, then all special commands will be available, but I'm not sure if it's worth the trouble.
UPD: ah, I see Thomas has been doing some work towards special commands is favorite queries in #600 so it's probably better to handle there.

Another concern is that I have changed the API of _multiline_exception (it now accepts the delimiter argument), which is one more difference from pgcli and the rest of the dbcli tools.

And generally, though having delimiter switching may be useful to people accustomed to the default mysql shell, it's primary purpose is to deal with stored procedures. Maybe it would be more straightforward to implement a simple call stack, so we can track BEGIN - END statements directly?

Checklist

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).

@codecov-io
Copy link

codecov-io commented Oct 5, 2019

Codecov Report

Merging #782 into master will increase coverage by 0.34%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
+ Coverage   77.94%   78.29%   +0.34%     
==========================================
  Files          25       26       +1     
  Lines        2403     2460      +57     
==========================================
+ Hits         1873     1926      +53     
- Misses        530      534       +4
Impacted Files Coverage Δ
mycli/packages/special/favoritequeries.py 86.95% <ø> (ø) ⬆️
mycli/sqlcompleter.py 88.74% <ø> (ø) ⬆️
mycli/packages/special/iocommands.py 89.35% <100%> (+0.42%) ⬆️
mycli/sqlexecute.py 87.11% <100%> (ø) ⬆️
mycli/clibuffer.py 68.42% <50%> (+1.75%) ⬆️
mycli/clitoolbar.py 36% <50%> (+1.21%) ⬆️
mycli/packages/special/delimitercommand.py 90.9% <90.9%> (ø)
mycli/main.py 76.68% <0%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d01061...1f99087. Read the comment docs.

@meeuw
Copy link
Contributor

meeuw commented Oct 7, 2019

  1. I think DELIMITER should be implemented in a class in mycli.packages.special and the delimiter should be stored there instead of passing it around though a lot of functions
  2. What is the actual use case for having a delimiter other than ; ?

Nice work though, it seems pretty thorough!

@amjith
Copy link
Member

amjith commented Oct 7, 2019

What is the actual use case for having a delimiter other than ; ?

The delimiter is changed from the default (;) when you want to create functions that have semi-colons in them.

This stackoverflow answer does a better job of explaining it.

https://stackoverflow.com/a/10259528

@amjith
Copy link
Member

amjith commented Oct 8, 2019

I think DELIMITER should be implemented in a class in mycli.packages.special and the delimiter should be stored there instead of passing it around though a lot of functions

@pasenor Do you want to take a stab at implementing it as part of the special commands package?

When I try out the feature I see DELIMITER listed twice in the completion menu. This is because it is listed as a keyword in sqlcompleter.py and also as a special command. You can remove the entry from the keyword in sqlcompleter.py file.

@gfrlv
Copy link
Member Author

gfrlv commented Oct 8, 2019

1. I think DELIMITER should be implemented in a class in mycli.packages.special and the 
delimiter should be stored there instead of passing it around though a lot of functions

Makes sense, but we still need to somehow let the is_multiline function know about the new delimiter. I'll take a closer look at special commands, thanks for the suggestion!

@gfrlv
Copy link
Member Author

gfrlv commented Oct 8, 2019

What is the actual use case for having a delimiter other than ; ?

As @amjith said, the only use case is declaring stored procedures with ; in them.
In fact, I feel it's an ugly solution, and what we need instead is a proper scoping based on BEGIN ... END, like what psql does. But a lot of scripts tailored for the default mysql shell already use DELIMITER, and it would be nice if mycli could run them.

@meeuw
Copy link
Contributor

meeuw commented Oct 8, 2019

mmm, I never used stored procedures in mysql, sounds like a valid use case...

@amjith
Copy link
Member

amjith commented Oct 13, 2019

This works great! I like how thorough the tests are done for this feature. 🥇

@amjith amjith merged commit ff42487 into master Oct 13, 2019
@amjith amjith deleted the delimiter-2 branch October 13, 2019 05:17
@meeuw
Copy link
Contributor

meeuw commented Oct 17, 2019

nicely done @pasenor !

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