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.

Sign up for GitHub

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

Jump to bottom

Add support to output enum definitions in the db doc README #601

Merged
merged 4 commits into from
Oct 6, 2024
Merged

Add support to output enum definitions in the db doc README #601

merged 4 commits into from
Oct 6, 2024

Conversation

insano10
Copy link
Contributor

I'd like enum support for the documentation.
It's not finished yet as there's no tests, but can someone give me feedback if I'm going down the right lines?
I'm only using Postgres at the moment which is why it's only been implemented for that driver.

When I build and run tbls against my local db, the enum output looks as expected in the README.md.

@fira42073
Copy link

Hey! I'm missing this feature a lot, and I guess not only me. What do we need to do to make this work?

@insano10
Copy link
Contributor Author

To be perfectly honest I was waiting for the maintainer to give an indication that if I worked on this, I would be given feedback and it would likely be merged. I don't have a lot of free time right now and didn't want to spend time fixing up tests only for it to sit there.

There does seem to be some desire for it though so if no one else wants to add it, I'll try pick it up soon.

@k1LoW
Copy link
Owner

k1LoW commented Sep 18, 2024 •
edited
Loading

@insano10 Thank you for your contribution! ( sorry for late reply 💦 )

I think it is a very good idea.

Let me give you just two pieces of feedback.

  1. if the Enum does not exist, it would be better not to mention anything in the documentation
  2. some tests seem to fail, many of them may succeed by fixing 1.

@k1LoW k1LoW added the enhancement New feature or request label Sep 18, 2024
@insano10 insano10 force-pushed the postgres-enums branch 2 times, most recently from 16b9cc2 to af18d5e Compare September 23, 2024 13:36
@insano10
Copy link
Contributor Author

@k1LoW
I believe I've got all the tests passing now.
I haven't been able to test all the different types of DBs, only against my local Postgres but it works well for that (for databases with/without enums).

k1LoW
k1LoW reviewed Oct 1, 2024
View reviewed changes
drivers/postgres/postgres.go Outdated Show resolved Hide resolved
@k1LoW
Copy link
Owner

k1LoW commented Oct 1, 2024

Please check the test results as well, as we have fixed the CI environment.

@insano10
Copy link
Contributor Author

insano10 commented Oct 4, 2024

@k1LoW Thank you for your feedback. I have fixed the test failures and updated the enum definitions to include their schema.

k1LoW
k1LoW reviewed Oct 4, 2024
View reviewed changes
drivers/postgres/postgres.go
}

enum := &schema.Enum{
Name: fmt.Sprintf("%s.%s", schemaName, enumName),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 NICE 👍

@insano10
Copy link
Contributor Author

insano10 commented Oct 4, 2024

@k1LoW Apologies I forgot to update the test expectation after adding the schema scoping. Fixed now.

dependabot bot and others added 4 commits October 5, 2024 14:40
@dependabot @insano10
chore(deps): bump the dependencies group with 8 updates
9e9a1a8 
Bumps the dependencies group with 8 updates:

| Package | From | To |
| --- | --- | --- |
| [cloud.google.com/go/bigquery](https://github.com/googleapis/google-cloud-go) | `1.62.0` | `1.63.0` |
| [cloud.google.com/go/spanner](https://github.com/googleapis/google-cloud-go) | `1.67.0` | `1.68.0` |
| [github.com/ClickHouse/clickhouse-go/v2](https://github.com/ClickHouse/clickhouse-go) | `2.28.2` | `2.29.0` |
| [github.com/mattn/go-sqlite3](https://github.com/mattn/go-sqlite3) | `1.14.22` | `1.14.23` |
| [go.mongodb.org/mongo-driver](https://github.com/mongodb/mongo-go-driver) | `1.16.1` | `1.17.0` |
| [golang.org/x/image](https://github.com/golang/image) | `0.19.0` | `0.20.0` |
| [golang.org/x/oauth2](https://github.com/golang/oauth2) | `0.22.0` | `0.23.0` |
| [google.golang.org/api](https://github.com/googleapis/google-api-go-client) | `0.191.0` | `0.197.0` |


Updates `cloud.google.com/go/bigquery` from 1.62.0 to 1.63.0
- [Release notes](https://github.com/googleapis/google-cloud-go/releases)
- [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md)
- [Commits](googleapis/google-cloud-go@spanner/v1.62.0...spanner/v1.63.0)

Updates `cloud.google.com/go/spanner` from 1.67.0 to 1.68.0
- [Release notes](https://github.com/googleapis/google-cloud-go/releases)
- [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md)
- [Commits](googleapis/google-cloud-go@spanner/v1.67.0...spanner/v1.68.0)

Updates `github.com/ClickHouse/clickhouse-go/v2` from 2.28.2 to 2.29.0
- [Release notes](https://github.com/ClickHouse/clickhouse-go/releases)
- [Changelog](https://github.com/ClickHouse/clickhouse-go/blob/main/CHANGELOG.md)
- [Commits](ClickHouse/clickhouse-go@v2.28.2...v2.29.0)

Updates `github.com/mattn/go-sqlite3` from 1.14.22 to 1.14.23
- [Release notes](https://github.com/mattn/go-sqlite3/releases)
- [Commits](mattn/go-sqlite3@v1.14.22...v1.14.23)

Updates `go.mongodb.org/mongo-driver` from 1.16.1 to 1.17.0
- [Release notes](https://github.com/mongodb/mongo-go-driver/releases)
- [Commits](mongodb/mongo-go-driver@v1.16.1...v1.17.0)

Updates `golang.org/x/image` from 0.19.0 to 0.20.0
- [Commits](golang/image@v0.19.0...v0.20.0)

Updates `golang.org/x/oauth2` from 0.22.0 to 0.23.0
- [Commits](golang/oauth2@v0.22.0...v0.23.0)

Updates `google.golang.org/api` from 0.191.0 to 0.197.0
- [Release notes](https://github.com/googleapis/google-api-go-client/releases)
- [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md)
- [Commits](googleapis/google-api-go-client@v0.191.0...v0.197.0)

---
updated-dependencies:
- dependency-name: cloud.google.com/go/bigquery
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: cloud.google.com/go/spanner
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: github.com/ClickHouse/clickhouse-go/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: github.com/mattn/go-sqlite3
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: go.mongodb.org/mongo-driver
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: golang.org/x/image
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: golang.org/x/oauth2
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: google.golang.org/api
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
...

Signed-off-by: dependabot[bot] <[email protected]>
@insano10
Add support to output Postgres enum definitions in the db doc README
138c698
@insano10
Fix postgres test expected README output
d898345
@insano10
Scope enums by their enclosing schema
20b1e62
@k1LoW
Copy link
Owner

k1LoW commented Oct 6, 2024

@insano10 Thank you!!!! GREAT!

@k1LoW k1LoW merged commit 76d30f8 into k1LoW:main Oct 6, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Oct 2, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Oct 11, 2024
Merged
@insano10
Copy link
Contributor Author

@k1LoW Thanks so much for shepherding this through. The setup-tbls github action has now picked up this release so my CI build output now includes enums 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k1LoW k1LoW k1LoW left review comments

Assignees
No one assigned
Labels
breaking-change enhancement New feature or request minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@insano10 @fira42073 @k1LoW