Skip to content

Add --no-optional-props #29

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

Closed
wants to merge 1 commit into from
Closed

Add --no-optional-props #29

wants to merge 1 commit into from

Conversation

andmis
Copy link

@andmis andmis commented Nov 1, 2022

By default, properties of models which have default values will be marked as optional in the generated TS interfaces. This makes sense if you think of the TS interfaces as defining what may legally be passed into the Python model's constructor. However, if you want the TS interfaces to represent the type of the output produced by calling .dict() on an instance of the model, you don't want the TS interface types' properties marked optional, since they will always be there.

This is particularly annoying when working with discriminated unions.

I don't think there is a general solution to this problem because the type accepted by the model constructor is actually different than the type emitted by .dict(), and you might want either one depending on your use case. So we add a command-line option making this behavior toggleable.

This PR should provide a clean work-around for #28.

@PeterBaker0
Copy link

@andmis

Will this CLI option supress genuine optional fields as well?

For example:
This python model

class OrganisationDomainInfo(BaseModel):
    # organisation info
    name: str

    # optional info
    ror: Optional[AnyHttpUrl]

Results in this interface:

export interface OrganisationDomainInfo {
  name: string;
  ror?: string;
}

which behaves perfectly.

Would applying the above CLI argument mean that this interface would become?

export interface OrganisationDomainInfo {
  name: string;
  ror: string;
}

if so, would it be possible to be more specific about which kind of overrides to apply. I would suggest this rule:

If a Pydantic model specifies a field which is a) not Optional[Type] and b) has a default value provided, the resulting interface should not have a possibly undefined field.

I realise this level of expressiveness may not be exposed through Pydantic - but putting my thoughts out there.

Thanks.

@andmis
Copy link
Author

andmis commented Nov 16, 2022

Actually this:

export interface OrganisationDomainInfo {
  name: string;
  ror?: string;
}

is not "the" interface for this:

class OrganisationDomainInfo(BaseModel):
    # organisation info
    name: str

    # optional info
    ror: Optional[AnyHttpUrl]

The reason is, it depends on whether you are specifying

  • the type of a dictionary that can legally be given to the class's constructor, or
  • the type of the dictionary returned by class_instance.dict(), for example when JSONifying.

If you want the latter behavior then the ror should not be marked optional in TS -- it will be present and set to None.

The type you gave accurately describes the dicts that can be passed into the class ctor. The type that class_instance.dict() will return is more accurately described as

export interface OrganisationDomainInfo {
  name: string;
  ror: string | undefined;
}

which is actually a different type in TS.

Admittedly this is not what my change --no-optional-props produces.


The real issue being addressed here is not actually Optional types on some of the class props, but optional arguments to the class ctor. If you have

class C(BaseModel):
  disc: Literal['C'] = 'C'
  n: int

then pydantic-to-typescript produces

interface C {
  disc?: 'C';
  n: int;
}

which is indeed the type of dict which may be expanded into C.__init__. What I want is to produce

interface C {
  disc: 'C';
  n: int;
}

which is the type returned by c.dict().


For example:

from typing import Optional
import pydantic
class C(pydantic.BaseModel):
  n: Optional[int]
  s: str
c = C(s="Hi")
c.dict()
# {'n': None, 's': 'Hi'}

@bibermann
Copy link

If you want the latter behavior then the ror should not be marked optional in TS -- it will be present and set to None.

The type you gave accurately describes the dicts that can be passed into the class ctor. The type that class_instance.dict() will return is more accurately described as

export interface OrganisationDomainInfo {
  name: string;
  ror: string | undefined;
}

Actually, the interface would need to look like

export interface OrganisationDomainInfo {
  name: string;
  ror: string | null;
}

But nullable fields are not supported by json-schema-to-typescript (there is a PR thought).

So you may consider working with "undefined only" in your API (by using model.json(exclude_none=True) and model.dict(exclude_none=True), for example). If you decide so, you would like to have this genuine optional field ror rather declared as ror?: string;. But this is out of scope of this PR and would go into another option, like --read-only-interfaces.

To clarify, this PR would define the optional ror as ror: string and if it's None in Python, it will be at JavaScript runtime either undefined or null, depending on the serialization argument exclude_none. So you would need to consider this case without type hints.

@avin-kavish
Copy link

Do you have a version of this you have deployed as a package?

@andmis
Copy link
Author

andmis commented Sep 13, 2023

Do you have a version of this you have deployed as a package?

No

@andmis andmis closed this by deleting the head repository Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants