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 auto generate conversion table to doc #5324

Merged
merged 13 commits into from
Apr 5, 2023

Conversation

hramezani
Copy link
Member

@hramezani hramezani commented Mar 31, 2023

Change Summary

Add auto-generate conversion table

This is the initial work to generate the conversion table automatically.
I've replaced the conversion table in v2 blog with {{ conversion_table }}. The conversion table rows are not complete yet and I will update the PR later.
Meanwhile, I am going to add missed tests as well.

I've built the docs locally and the new build_conversion_table works fine and generates the conversion table as expected.

Related issue number

Fixes #4680

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise looking good.

I guess we need to add more types before we see how this really works. In particular, do we need more information to describe generic types?

| `is_instance` | - | both | JSON | never valid |
| `callable` | `Any` | both | python | `callable()` check returns `True` |
| `callable` | - | both | JSON | never valid |
{{ conversion_table }}
Copy link
Member

Choose a reason for hiding this comment

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

Probably best to leave the one in the blog post unchanged.

Instead a new page to usage with the conversion table.

We'll need to add a big heading to this blog post that it's superseded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably best to leave the one in the blog post unchanged.

Reverted

Instead a new page to usage with the conversion table.

A new page was added as usage/conversion_table.md

We'll need to add a big heading to this blog post that it's superseded.

I've made the existing sentence bold.

pic

class Row:
field_type: type[Any]
input_type: type[Any]
mode: Literal['lax', 'strict', 'both']
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode: Literal['lax', 'strict', 'both']
mode: Literal['lax', 'strict']

"strict" should always imply "lax" as well, if there are exceptions, we can add specific comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. both removed

'lax',
'python',
condition='assumes UTF-8, error on unicode decoding error',
valid_examples=[b's'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
valid_examples=[b's'],
valid_examples=[b'this is bytes'],

probably good to change the others to something more readable too.

Copy link
Member Author

Choose a reason for hiding this comment

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

All changed

field_type: type[Any]
input_type: type[Any]
mode: Literal['lax', 'strict', 'both']
input_format: Literal['python', 'JSON', 'python, JSON']
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
input_format: Literal['python', 'JSON', 'python, JSON']
input_format: Literal['python', 'JSON', 'python & JSON']

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

'both',
'python, JSON',
condition='max abs value 2^64 - i64 is used internally',
core_schema=core_schema.IntSchema,
Copy link
Member

Choose a reason for hiding this comment

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

can you add an invalid example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an invalid example. Also, I've separated the strict and lax mode for int type in table because it raises ValidationError for number > limit in strict mode but it will limit the input to the max in lax mode.
I've added descriptions for these in the condition field but I am not sure about the wording.

I've added tests for both above-mentioned cases.

core_schema: type[CoreSchema] = None


table_infos: list[Row] = [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
table_infos: list[Row] = [
table: list[Row] = [

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

'Defined in',
]


def md2html(s: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

we might want other things, can we use the proper markdown -> html library from mkdocs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not very familiar with mkdocs and it's the related library.
@tpdorsey Do you have an idea?

Copy link
Member

Choose a reason for hiding this comment

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

if this isn't easy, keep it as-is, at least for now.

@samuelcolvin
Copy link
Member

please update.

@pydantic-hooky pydantic-hooky bot added the awaiting author revision awaiting changes from the PR author label Apr 1, 2023
@hramezani hramezani force-pushed the conversion_table branch 5 times, most recently from a89e3fd to f6e9ab6 Compare April 3, 2023 11:37
core_schema: type[CoreSchema] = None


table: list[Row] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

@samuelcolvin
The row list will be distributed as JSON lines or another format? in the wheel.
Does the list affect the behavior of the strict argument? Or does the list explain the behavior pydantic-core?
Also, Does the user custom type use the Row class?

Copy link
Member

Choose a reason for hiding this comment

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

if including the JSON in the wheel/sdist would help you, we definitely can.

Yes, the strictargument is relevant here, that logic is mostly implemented in pydantic-core, some is implemented in types in pydantic.

There's currently no way for user custom types to provide Row information.

Copy link
Contributor

Choose a reason for hiding this comment

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

if including the JSON in the wheel/sdist would help you, we definitely can.

Thank you. I will pick up the JSON from the package.

Yes, the strictargument is relevant here, that logic is mostly implemented in pydantic-core, some is implemented in types in pydantic.

I see.

There's currently no way for user custom types to provide Row information.

How can I get the expected type of user custom types?
I don't need the answer now. But, we should be clear sometime.

@hramezani hramezani force-pushed the conversion_table branch 3 times, most recently from 5bc1742 to 74fa64c Compare April 4, 2023 08:59
Row(
date,
date,
'stric',
Copy link
Contributor

Choose a reason for hiding this comment

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

Strict

Row(
None,
None,
'stric',
Copy link
Contributor

Choose a reason for hiding this comment

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

Strict

datetime,
'lax',
'python',
condition='must be exact date, eg. no H, M, S, f',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
condition='must be exact date, eg. no H, M, S, f',
condition='must be exact date, eg. no `H`, `M`, `S`, `f`',

'lax',
'python & JSON',
condition=(
'interpreted as seconds or ms from epoch, '
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'interpreted as seconds or ms from epoch, '
'Interpreted as seconds or ms from epoch. '

'python & JSON',
condition=(
'interpreted as seconds or ms from epoch, '
'see [speedate](https://docs.rs/speedate/latest/speedate/), must be exact date'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'see [speedate](https://docs.rs/speedate/latest/speedate/), must be exact date'
'See [speedate](https://docs.rs/speedate/latest/speedate/). Must be exact date.'

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest applying same change to equivalent conditions.

str,
'strict',
'JSON',
condition='format YYYY-MM-DDTHH:MM:SS.f etc. see [speedate](https://docs.rs/speedate/latest/speedate/)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
condition='format YYYY-MM-DDTHH:MM:SS.f etc. see [speedate](https://docs.rs/speedate/latest/speedate/)',
condition='Format: `YYYY-MM-DDTHH:MM:SS.f`. See [speedate](https://docs.rs/speedate/latest/speedate/).',

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, apply same edit to similar conditions.

Any,
'strict',
'python',
condition='`isinstance()` check returns True',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
condition='`isinstance()` check returns True',
condition='`isinstance()` check returns `True`.',

@hramezani
Copy link
Member Author

Thanks @tpdorsey, All addressed. please review

@pydantic-hooky pydantic-hooky bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Apr 5, 2023
@pydantic-hooky pydantic-hooky bot assigned adriangb and unassigned hramezani Apr 5, 2023
@hramezani hramezani assigned tpdorsey and unassigned adriangb Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V2 docs: Conversion table
5 participants