-
Notifications
You must be signed in to change notification settings - Fork 30
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 avro doc field of a record #90
base: main
Are you sure you want to change the base?
Conversation
…of-arrays Add support for array of arrays
Update build-and-publish.yml
Update build-and-publish.yml
Add support for pydantic v2
@timvancann You merged the branch where sharonyogev worked on to support pydantic v2 and force pushed. If you were thinking of merging together, please review this pr. 🙇 |
@sookyomkim I expect I made a booboo somewhere. Removing the pydantic-v2 changes was not intended. These changes are back so feel free to rebase on main. After that we can approve and merge this one :) |
So I would like to give my 2 cents here. I am a bit hesitant to automatically add description based on docstring. What would happen for different docstring formats? Some people might add types and param description in docstring as well how would these then translate to decriptions in the avro file? To still support description I would suggest perhaps:
|
Take a look at this pydantic code. Shouldn't that be enough? Do we need to worry about the format of the docstring? Isn't it enough to see it as text and just put it in? Initially, I implemented it by using pydantic json schema's description, but the reason for using a direct docstring is that pydantic description takes the docstring of the parent class if the child class does not have a docstring. |
Perhaps a good balance would be taking only the first section of a docstring, as the convention of what they should look like is fairly well defined: https://peps.python.org/pep-0257/#handling-docstring-indentation
|
if cls.__doc__: | ||
avro_schema_dict["doc"] = cleandoc(cls.__doc__) |
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.
if cls.__doc__
already acts as the necessary check for whether or not schema['description']
both exists and wasn't inherited.
So simply:
if cls.__doc__: | |
avro_schema_dict["doc"] = cleandoc(cls.__doc__) | |
if cls.__doc__: | |
avro_schema_dict["doc"] = schema["description"] |
Would work as desired~
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.
You're right, but if we use schema["description"] directly, it will get the docstring of the parent class if the child class's docstring isn't defined.
pydantic uses inspect.getdoc().
So if we don't define a docstring, we'll get AvroBase's docstring.
That's why i used cls.__doc__ directly instead of using schema["description"].
pydantic V2 doesn't have this problem. It only gets the docstring of the child class.
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.
My point is,cls.__doc__
will be None
for both V1 and V2 if not directly defined.
__doc__
is not inherited in either case, and only the description is inherited in V1.
So if __doc__
is defined, there's no reason to redo what pydantic does, and if __doc__
isn't defined the line gets skipped in either case...
I hadn't seen this PR until after I made mine, and originally closed it. |
add avro doc field of a record using model's docstring