joereynolds / SQHell.vim

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

Feature/insert row key #44

Open jbyte opened 6 years ago

jbyte commented 6 years ago

Description (what is it you've done, has anything changed?)

Checklist

jbyte commented 6 years ago

Well I also fixed the travis issue where it didn't use the correct branch/pull request for Vader tests. Although the mac tests still fail because the macOS sort works different than the linux one.

joereynolds commented 6 years ago

Hey @jbyte

All looks good, I'm going to fix the Mac Vs Linux sort bug before merging any pull requests

Once I've done that I'll comment on here

jbyte commented 6 years ago

I'm fine with that.

joereynolds commented 6 years ago

Hey @jbyte,

I've fixed the tests/code, can you pull down master and try again?

jbyte commented 6 years ago

The tests are fine now 👍

joereynolds commented 6 years ago

When I try to ZZ on an i in the SQHResult buffer with this data

id,prefix,location,date,artist,url
"22", "With", "Wales", "2018-07-03", "The Three Stooges", "http://google.com"

I get

Incorrect number of values. Expected: 6, got: 11.

jbyte commented 6 years ago

It should be fixed now.

joereynolds commented 6 years ago

I think now that we do everything in one buffer, your calls to :bd can be removed otherwise we get some undesirable jumps. For example if I do the following

SQHDatabase -> SQHTable -> SQHResult -> e to edit result -> <c-o>

Instead of going back to the SQHResult as expected, it goes to SQHTable

This also happens on the i to insert a new record.

Looking good so far though :D

jbyte commented 6 years ago

Well I wouldn't remove them, but change them. Because they are used for "reloading" the buffer. Example: SQHResult -> e to edit -> edit data -> ZZ to save and close -> updated SQHResult I'll just change the :bd to something like dG and put newresults

Edit: Never mind it's fine to remove them, I'll just move the "reloading" functionality to ZZ in SQHResult

Edit2: The :bd in mysql#EditRow and mysql#InsertRow should be changed and moved to ZZ. The :bd in mysql#DropDatabase and mysql#DropTableFromDatabase should be changed in place.

But in the end I think this would need quite a bit of refactoring so maybe should be its own PR?

jbyte commented 6 years ago

Thoughts on the refactoring: #46.

joereynolds commented 6 years ago

Sounds good to me, we can test the query generation better this way too.

Do we need to delete all lines in the buffer each time? We use enew! inside the InsertResultsToNewBuffer function anyway so the previous buffer would be removed (though still available in the jump list I guess).

Also it sounds like the pros outweight the cons so feel free to take a stab it, I would definitely like a cleaner codebase for this