joereynolds / SQHell.vim

An SQL wrapper for vim
MIT License
131 stars 9 forks source link

Add 'dd' keybinding for deleting rows #25

Closed jbyte closed 6 years ago

jbyte commented 6 years ago

Implemented enhancement from issue #8.

Functionality:

sqhell#GetColumnName and mysql#DeleteRow detail explanation: Because we can't know by what column the user wants do delete row(s), you can choose it with the cursor.

Example:

| Attr1 | Attr2 |
+------+-------+
|      1 | test1 |
|      2 | test2 |
|      3 | test2 |
+------+-------+

If your cursor is anywhere from the first | to the (1, 2, or 3), the executed query will be: DELETE FROM database.table WHERE Attr1="<number>"

If your cursor is anywhere from the second | to the (test1, test2, test2), the executed query will be: DELETE FROM database.table WHERE Attr2="<string>"

Basically when pressing 'dd' it saves the current cursor position and then moves the cursor to the first line (does not change the column) and gets the column name to use for the DELETE statement.

I hope the explanation is good enough.

joereynolds commented 6 years ago

Hey @jbyte

I like the idea, is there any chance you can pull in from master? I've extracted some more code out of vendor specific files and into the main autoload/sqhell.vim file.

Also when you test this, can you make sure it works with

SQHExecuteCommand SQHExecuteLine SQHExecuteFile SQHExecuteBlock

Better yet, if you could write some tests (under test dir), the codebase will love you for it

Thanks!

jbyte commented 6 years ago

Hey @joereynolds,

I've pulled the changes from master and already pushed them to the feature branch. But for the tests I'm not sure how to do it for the functions you listed. As far as I know those functions operate on user created queries. So I would like to know what exactly did you mean?

joereynolds commented 6 years ago

Hey jbyte,

For the functions listed a quick manual test will do. I plan to refactor them a bit to make them more testable.

One of the main things to start refactoring is the building of the query. i.e. We should have a function that constructs the SQL string and returns it, then we can write tests against that and make sure it is what we expect.

In the meantime, don't worry about writing tests for those but going forwards, the more tests the better! I've written up a CONTRIBUTING.md file for people considering making a pr.

Thanks!

joereynolds commented 6 years ago

Also you've said you've pushed those changes to the feature branch but I can still see mysql#ExecuteCommand and friends which are now housed inside autoload/sqhell.vim :)

jbyte commented 6 years ago

Oh, sorry I rebased the feature branch onto your master and while the execute functions were added to the autoload/sqhell.vim, the ones in the autoload/mysql.vim were not removed. Fixed now :).

joereynolds commented 6 years ago

Just trying it now, is it possible to just get instead of all the crud that comes along with it?

image

Also you're trying to call mysql#ExecuteCommand in your code which is now inside the autoload/sqhell directory

jbyte commented 6 years ago

Ok, fixed so it uses the correct function. Can I ask where is your cursor in the above image when calling :call mysql#DeleteRow(expand('<cword>'))?

joereynolds commented 6 years ago

It's on the pipe symbols

image

Even still, this should just grab the id not the entire row I would've thought?

jbyte commented 6 years ago

Yeah, that's how its supposed to work... tmp It works fine for me. Whats the first line in your buffer?

joereynolds commented 6 years ago

This was how I managed to reproduce it. Note that I go through databases and tables first

jbyte

jbyte commented 6 years ago

Oh, I understand. We probably have different mysql versions or packages (what OS do you run?). Our database outputs are not the same. Mine:

|      id   |      tmp |
+-----------+----------+

Your:

+------------+---------+
|      id    |     tmp |
+------------+---------+

Mine basically doesn't have the first row. I'll make it work with both versions.

jbyte commented 6 years ago

It should be correct now. I tested with my buffer modified so it looks like yours so it should work.

joereynolds commented 6 years ago

ah okay, I think I know what this is. You might want to take a look at mysql#PostBufferFormat(), it deletes 2 lines from the top. Mainly because I get an insecure password notice and a blank line, that function fixes that.

joereynolds commented 6 years ago

Hey @jbyte,

Back with a few more issues :( It's looking way better but it's referencing the wrong column names sometimes.

Here's a gif

jbyte-2

I wouldn't blame you if you think this is more trouble than it's worth :smile:

(Sorry about the gif speed)

jbyte commented 6 years ago

Nope I don't think it will be really hard to fix. When selecting the column name it just moves the cursor to the first (or second depending on the output) line with preserving the column number. I'll just make it move the column to the previous '|' symbol and it should work.

jbyte commented 6 years ago

It also has a problem when the cursor is on a database value that is composed of multiple whitespace separated words. It just takes the word under the cursor not the whole string. I'll try to fix it.

jbyte commented 6 years ago

I think it should be fixed now.

joereynolds commented 6 years ago

I'll test this tomorrow, thanks for the constant stream of PRs that you're providing!

joereynolds commented 6 years ago

Lookin' good!