8000 📝 Add docstrings to `types.py` by Kludex · Pull Request #6091 · pydantic/pydantic · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@Kludex
Copy link
Member
@Kludex Kludex commented Jun 12, 2023

Closes DOC-18

Selected Reviewer: @hramezani

@linear
Copy link
linear bot commented Jun 12, 2023

@Kludex Kludex requested a review from lig June 12, 2023 11:14
@Kludex
Copy link
Member Author
Kludex commented Jun 12, 2023

please review

@cloudflare-workers-and-pages
Copy link
cloudflare-workers-and-pages bot commented Jun 12, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d20f8f7
Status: ✅  Deploy successful!
Preview URL: https://6f8c745b.pydantic-docs2.pages.dev
Branch Preview URL: https://kludex-doc-18-types.pydantic-docs2.pages.dev

View logs

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Jun 12, 2023
@pydantic-hooky pydantic-hooky bot assigned Kludex and unassigned hramezani Jun 12, 2023
@Kludex Kludex requested a review from lig June 12, 2023 11:23
@classmethod
def decode(cls, data: bytes) -> bytes:
"""Can throw `PydanticCustomError`."""
"""Decode the data using the encoder.
Copy link
Member

Choose a reason for hiding this comment

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

Decoder?

Suggested change
"""Decode the data using the encoder.
"""Decode the data using the decoder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, actually, the EncoderProtocol defines how to decode and encode. 🤔

@lig what do you want me to write here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just left "Decode the data" and "Encode the data" for the opposite method. This is a protocol definition and it's up to user to decide what to use for the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are not willing to change the EncoderProtocol name, I think it's better if we maintain the "encoder" here. 🤔

8000

Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Copy link
Member
@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

Great addition. I left some suggestions.
Other than that LGTM 👍

@Kludex Kludex merged commit 650ec0b into main Jun 12, 2023
@Kludex Kludex deleted the kludex/DOC-18-types branch June 12, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting author revision awaiting changes from the PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0