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.

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

introspect column comments #842

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

maktouch
Copy link
Contributor

closes #841

Copy link

vercel bot commented Jan 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2024 1:43pm

@maktouch maktouch marked this pull request as ready for review January 11, 2024 21:21
@koskimas
Copy link
Member

koskimas commented Jan 12, 2024

Thank you!

How about SQLite and mssql? If they support comments, this feature needs to be implemented for those also.

Tests are also needed for every feature. Add at least one test that adds a comment and checks that it gets returned by the introspector. Currently the tests and the whole feature are completely broken.

@maktouch
Copy link
Contributor Author

Thank you!

How about SQLite and mssql? If they support comments, this feature needs to be implemented for those also.

Tests are also needed for every feature. Add at least one test that adds a comment and checks that it gets returned by the introspector. Currently the tests and the whole feature are completely broken.

Haha it's why I made this field optional. I wasn't familiar enough with them to implement it 😂

Ok I'll get familiar, implement the feature, make it non optional but billable, and do the tests.

@koskimas
Copy link
Member

Haha it's why I made this field optional. I wasn't familiar enough with them to implement it

It should still be optional even if all built-in dialects support it. Not all databases do, and we want 3rd party dialects to be able to implement the API.

@maktouch
Copy link
Contributor Author

Haha it's why I made this field optional. I wasn't familiar enough with them to implement it

It should still be optional even if all built-in dialects support it. Not all databases do, and we want 3rd party dialects to be able to implement the API.

ok that makes sense, so I'll make the value be undefined if it's empty

@maktouch
Copy link
Contributor Author

maktouch commented Jan 12, 2024

I've reverted my changes and started from scratch in TDD mode

  • The first thing I did was modify test/node/src/test-setup.ts to add a comment somewhere for each dialect. Unfortunately, I wasn't able to reliably add a comment for sqlite here, so sqlite always returns undefined for comment. If you can help me out here, that would be great! (or we can also just ignore it, it doesn't seem like comment is a real feature in sqlite)
  • Then I added the expected comments in test/node/src/schema.test.ts and test/node/src/introspect.test.ts
  • Implemented the features in src/dialect/$DB/$DB-introspector.ts until it passed.

Let me know what else I can change! Thanks! :)

CleanShot 2024-01-12 at 10 24 00

@maktouch maktouch changed the title introspect column comments for mysql and postgres introspect column comments Jan 12, 2024
@maktouch maktouch force-pushed the introspect-column-comments branch from b2dd8a3 to c483477 Compare January 13, 2024 14:09
@maktouch
Copy link
Contributor Author

Hey @koskimas

Please check this PR when you have time 🙏🙏

Thank you

@maktouch maktouch requested a review from koskimas January 18, 2024 22:37
Copy link
Member

@koskimas koskimas left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you 👍

@koskimas koskimas merged commit cd918dc into kysely-org:master Jan 20, 2024
5 checks passed
@igalklebanov igalklebanov added enhancement New feature or request mysql Related to MySQL sqlite Related to sqlite postgres Related to PostgreSQL built-in dialect Related to a built-in dialect mssql Related to MS SQL Server (MSSQL) labels Mar 11, 2024
thecodrr pushed a commit to thecodrr/kysely that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
built-in dialect Related to a built-in dialect enhancement New feature or request mssql Related to MS SQL Server (MSSQL) mysql Related to MySQL postgres Related to PostgreSQL sqlite Related to sqlite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introspect should also fetch column comments so we can generate typescript comments
3 participants