search

kndndrj / nvim-dbee

Interactive database client for neovim
GNU General Public License v3.0
617 stars 40 forks source link

enhancement: Redshift #34

Closed MattiasMTS closed 10 months ago

MattiasMTS commented 10 months ago

👷 What has changed 👷

The focus of this PR is to open up this awesome plugin for Redshift users!

I'm opening this PR as a draft for early review. I'll push some more changes with respect to tests, documentation, etc

Redshift

Layout

Logging

Extended the logging library with formatted functions and refactored a few use cases where this was needed.

Bash

I've shellfmt as formatter on nvim and I was in build.sh, which led to the formatted version. Let me know if you want me to revert this. Perhaps it is more worth adding a pre-commit-config.yaml to fix this for future stuff. We can add more configs with respect to sh, go, lua, etc.

kndndrj commented 10 months ago

Hey @MattiasMTS thanks for the contribution!

The changes look nice - don't worry about fomatters (for bash I used something different - beutysh or something like that, but it doesn't matter)

The only thing which I'd like to ask you is that I just prepared a PR for database switching, so I'd like to ask you to revisit this PR once that is merged (I'll try to ping you here) It shouldn't be too complicated, but I expect a few merge confilicts.

MattiasMTS commented 10 months ago

Hey @MattiasMTS thanks for the contribution!

The changes look nice - don't worry about fomatters (for bash I used something different - beutysh or something like that, but it doesn't matter)

The only thing which I'd like to ask you is that I just prepared a PR for database switching, so I'd like to ask you to revisit this PR once that is merged (I'll try to ping you here) It shouldn't be too complicated, but I expect a few merge confilicts.

Sounds great! I had a gut feeling this was coming up, hence the draft PR.

I'll hold and rebase after you've merged database switching. Until then I'll pick up something in the backlog/issue.

Thanks for fast response! :)

kndndrj commented 10 months ago

Here it is. I merged into master.

Until then I'll pick up something in the backlog/issue

whoa, nice :)

Also feel free to ask questions, I'd be happy to help :)

MattiasMTS commented 10 months ago

Here it is. I merged into master.

Until then I'll pick up something in the backlog/issue

whoa, nice :)

Also feel free to ask questions, I'd be happy to help :)

Sweet. I'll take a peak and rebase. 😋

MattiasMTS commented 10 months ago

This should now be ready for "real review". Thanks for the initial feedback @kndndrj ! I addressed the rebasing issues, main thing was the new list helper method which in this case needed to accept the vars argument that the helper:get uses as well in order to list the option difference for e.g. VIEW, TABLE, etc. Perhaps worth to add e.g. "Functions" here in the future.

client.go

NB I did some extension of the client.go file, which is to add the interfaces DatabaseClient and DatabaseConnetion. Also updated the return statement of some methods to return interfaces rather than the concrete result. It isn't idiomatic that methods return interfaces rather than the concrete stuff but this refactoring facilitated testing.

The interfaces were needed in order to add mock structs, see how I did it in the redshift_test.go file. Maybe this isn't the prettiest way of doing it. Let me know if you have any other ideas on how to do this and I'm more than happy to do it :)

history.go & result.go

Added SetCallBack and SetCustomHeader interface methods to. Updated the HistoryRows struct with placeholder method to fulfil the interface of models.IterResult. These were needed due to the return statement of the results now returning interfaces rather than the concrete stuff. Again, this made testing easier. Perhaps not the best way to do it since large interfaces become difficult to handle in the long run.. :/

kndndrj commented 10 months ago

I did a quick review from my phone, and overall I like the changes - reminds me of some stuff that I forgot to clean up myself. I'd still like to take a closer look when I have some time though.

Thanks for the contribution and a detailed overview :)

MattiasMTS commented 10 months ago

Looking forward to it!

Thanks for awesome response and feedback during these PRs 😄

MattiasMTS commented 10 months ago

I'll add this as a reminder to myself to actually reconsider using the redshiftdata api https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/redshiftdata rather than using the database/sql package.

kndndrj commented 10 months ago

thanks for the contribution. I tested some stuff and fixed what didn't work and added some extra stuff.