enhancement: Redshift by MattiasMTS · Pull Request #34 · kndndrj/nvim-dbee

MattiasMTS · 2023-08-23T21:33:06Z

👷 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

Added a new client with a similar structure as the Postgres client but with some differences. Refactored the Layout schema for both clients to re-usable function.
Extended the layout to make use of VIEWS
Added more code actions to action 1 for both VIEWS and TABLES
Made upper case letters for keywords in helpers

Layout

added sorting with respect to history to get the latest query at the top and alphabetical on the schema

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 · 2023-08-24T05:12:46Z

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 · 2023-08-24T06:45:29Z

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 · 2023-08-24T18:54:13Z

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 · 2023-08-24T18:55:20Z

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. 😋

- improve json marshaling and add schemaless support - improve mongo and redis output support

MattiasMTS · 2023-08-25T18:42:25Z

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 · 2023-08-25T20:37:51Z

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 · 2023-08-25T20:58:39Z

Looking forward to it!

Thanks for awesome response and feedback during these PRs 😄

MattiasMTS · 2023-08-31T11:25:09Z

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

Thanks again for submitting this PR and sorry that it took me so long to do a propper review.

I'd be really thankful if you address the changes, so we can merge this cool feature!

kndndrj · 2023-09-01T17:12:52Z

ci/build.sh

@@ -4,37 +4,44 @@
 set -e


Nice of you that you want to format the script, but it was formatter with a different formatter already 😅

I think we can remove these changes if you agree.

Deleted the commit. Totally agree. Is it worth adding some pre-commit-config in the future? 😋

It would make sense, yeah. although I'm not the most familiar with those configs. If you want, you can come up with something

dbee/clients/common/client.go

dbee/clients/redshift.go

dbee/models/result.go

dbee/vim/logger.go

lua/dbee/config.lua

lua/dbee/handler/init.lua

lua/dbee/handler/conn.lua

This reverts commit 36fb188.

kndndrj

Except for the 2 minor nitpicks, I think the PR looks fine now.
I'll do a quick test and merge it today or tomorrow. Thanks again!

kndndrj · 2023-09-02T14:22:48Z

dbee/conn/history.go

+}
+
+func (r *HistoryRows) SetCustomHeader(header models.Header) {
+	// no-op


Are these methods an artifact of the deleted interface?

Indeed they are c82b85e

kndndrj · 2023-09-02T14:23:53Z

lua/dbee/handler/conn.lua

+    ---@param children _LayoutGo[] -- children to sort
+    ---@param field string -- field to sort by
+    ---@param ascending bool -- set to true to sort ascendingly
+    local function sortChildrenByField(children, field, ascending)


cool, just snake case please 😅

15e5262 sorry, too much scala

- clean up postgres/redshift layout getter - small cleanup

kndndrj · 2023-09-06T17:29:37Z

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

Mattias Sjödin and others added 2 commits June 16, 2023 23:01

refactor: use pg_views instead of pg_matviews

36fb188

Merge branch 'kndndrj:master' into master

2ef4864

MattiasMTS and others added 3 commits August 25, 2023 10:27

Merge branch 'kndndrj:master' into master

177bca3

- add schemaful or schmaless type to metadata

3457a80

- improve json marshaling and add schemaless support - improve mongo and redis output support

refactor: move validate part to validate_config function

98853a6

MattiasMTS marked this pull request as ready for review August 25, 2023 18:32

kndndrj requested changes Sep 1, 2023

View reviewed changes

MattiasMTS added 15 commits September 2, 2023 12:21

feat: extend loggers with format

f954f31

feat: add LayoutView

296dd9b

refactor: restructure struct fields to reduce size

21f916a

feat: add redshift client

aaaaee9

refactor: make use of layout func

80324ec

feat: add view icon

8e70ef3

maint: remove empty helpers.lua

827090c

feat: add action 1 for Redshift

7ae71be

maint: upper case sql keywords

5567476

maint: remove print("here") statement

7f9c719

maint: add placeholder for text_highlight to stop linting issue

26eae92

maint: add todo comment on ListHistory

78f854b

refactor: use logger format and layoutErrString

d7bf8ff

refactor: sorting of latest history query

34b6b97

Revert "refactor: use pg_views instead of pg_matviews"

138e628

This reverts commit 36fb188.

MattiasMTS and others added 20 commits September 2, 2023 12:21

maint: delete rebase files

1fee033

maint: fix layout conflict hunks

c4abba3

refactor: use url parser for rawURL

2bece40

refactor: use new Layout enums

a407790

refactor: fix helper action for different materialization

be9269e

refactor: add interfaces for client.go for easier testing

068394e

refactor: LayoutView -> LayoutTypeView

67e86a6

refactor: use common.DatabaseClient

9d2c6f8

tests: first stab at tests

87a150c

refactor: syntax for err statements

fb0f1f8

tests: add remaining tests

492f120

maint: add docstrings

84f8aaf

fix: add schema path for postgres List helper

3d2a58d

refactor: move and rename fetchPsqlLayouts

c1b18cb

refactor: sqlfluff and indentation

120c3f2

refactor: sorting of schemas/types/names

36bea65

maint: cleanup unused stuff

c05c4a3

revert: add back unintentional removed TODOs

02ed575

revert: changes to result.go and client.go

b9d0035

refactor: remove tests and add back common.Client

d91671b

MattiasMTS requested a review from kndndrj September 2, 2023 13:58

kndndrj reviewed Sep 2, 2023

View reviewed changes

Mattias Sjödin and others added 3 commits September 2, 2023 16:28

maint: remove placeholders

c82b85e

refactor: snake case instead of camel case

15e5262

- fix sorting - add sorting option to go layout

4a97e33

- clean up postgres/redshift layout getter - small cleanup

kndndrj merged commit 0261046 into kndndrj:master Sep 6, 2023

Conversation

MattiasMTS commented Aug 23, 2023

👷 What has changed 👷

Redshift

Layout

Logging

Bash

Uh oh!

kndndrj commented Aug 24, 2023

Uh oh!

MattiasMTS commented Aug 24, 2023

Uh oh!

kndndrj commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattiasMTS commented Aug 24, 2023

Uh oh!

MattiasMTS commented Aug 25, 2023

client.go

history.go & result.go

Uh oh!

kndndrj commented Aug 25, 2023

Uh oh!

MattiasMTS commented Aug 25, 2023

Uh oh!

MattiasMTS commented Aug 31, 2023

Uh oh!

kndndrj left a comment

Choose a reason for hiding this comment

Uh oh!

kndndrj Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

MattiasMTS Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

kndndrj Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kndndrj left a comment

Choose a reason for hiding this comment

Uh oh!

kndndrj Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

MattiasMTS Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

kndndrj Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

MattiasMTS Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

kndndrj commented Sep 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kndndrj commented Aug 24, 2023 •

edited

Loading

`client.go`

`history.go` & `result.go`