-
Notifications
You must be signed in to change notification settings - Fork 280
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
introspect column comments #842
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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. |
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 |
4e14fc1
to
b2dd8a3
Compare
I've reverted my changes and started from scratch in TDD mode
Let me know what else I can change! Thanks! :) |
b2dd8a3
to
c483477
Compare
Signed-off-by: Makara Sok <[email protected]>
c483477
to
50e4516
Compare
Hey @koskimas Please check this PR when you have time 🙏🙏 Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thank you 👍
…ysely-org#842) Signed-off-by: Makara Sok <[email protected]>
closes #841