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

✨ Add field-level db comments when a field description exists #1293

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

iloveitaly
Copy link

This PR fixes #492 (comment) by adding column-level db comments
when a field description exists.

I can add some tests if this is the sort of PR that would be accepted.

I've implemented this in my SQLModel extension here: https://github.com/iloveitaly/activemodel/blob/master/activemodel/get_column_from_field_patch.py

Details:

  • feat: pull field descriptions into db comments
  • docs: document autostring
  • feat: add use_attribute_docstrings pydantic config for improved docstrings

@svlandeg svlandeg added the feature New feature or request label Feb 20, 2025
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution!

There are some linting errors and test failures - could you look into those? It will facilitate the review process if the CI is green. I'll go ahead and put this PR in draft in the meantime 🙏

@svlandeg svlandeg marked this pull request as draft February 20, 2025 10:42
@svlandeg svlandeg changed the title feat: add field-level db comments when a field description exists ✨ Add field-level db comments when a field description exists Feb 20, 2025
@iloveitaly
Copy link
Author

@svlandeg awesome, will work on CI and we can go from there! Is there anything else that I should work on in order to get this merged?

@svlandeg
Copy link
Member

@svlandeg awesome, will work on CI and we can go from there! Is there anything else that I should work on in order to get this merged?

A test case is always useful, as it helps the reviewer as well to quickly run a small example. And it'll likely be necessary anyway to get the test coverage back to 100% (ensuring that all code in the code base gets tested).

@iloveitaly
Copy link
Author

iloveitaly commented Feb 26, 2025

Awesome, I should have some time to help there. I have a bunch of other SQLModel improvements I'd love to pull over from activemodel, so understanding the best contribution flow would be really helpful

@svlandeg
Copy link
Member

Awesome, I should have some time to help there. I have a bunch of other SQLModel improvements I'd love to pull over from activemodel, so understanding the best contribution flow would be really helpful

Sure thing, let me know if you have specific questions/doubts about contributing to sqlmodel, happy to provide guidance if needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add comments on tables and columns, visible also on the generated DB
2 participants