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

Change ordering of actions #1961

Merged
merged 9 commits into from
Sep 26, 2024
Merged

Change ordering of actions #1961

merged 9 commits into from
Sep 26, 2024

Conversation

prmshepherd
Copy link
Contributor

Update the ordering of the actions as the one literal on default option is not working for descriminator properties

Update the ordering of the actions as the one literal on default option is not working for descriminator properties
Copy link

codecov bot commented Jun 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (83ebb6d) to head (8bd40ef).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1961   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           38        38           
  Lines         4205      4205           
  Branches       976       976           
=========================================
  Hits          4205      4205           
Flag Coverage Δ
unittests 99.66% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Jun 1, 2024

CodSpeed Performance Report

Merging #1961 will not alter performance

Comparing prmshepherd:main (8bd40ef) with main (83ebb6d)

Summary

✅ 31 untouched benchmarks

@koxudaxi
Copy link
Owner

koxudaxi commented Jun 5, 2024

@prmshepherd
Sorry for my late response.
Thank you for creating the PR.
Could you please provide the test case?
I would like to add the unittest for the changes.

@prmshepherd
Copy link
Contributor Author

Hi @koxudaxi, sorry I have this in draft as more work is needed by me. My use-case is when I am using a discriminator

openapi: 3.0.0
info:
  title: Pet API
  version: 1.0.0
paths:
  /pets:
    post:
      summary: Add a new pet
      requestBody:
        required: true
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Pet'
      responses:
        '200':
          description: Successfully added the pet
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pet'
components:
  schemas:
    Pet:
      type: object
      oneOf:
       - $ref: '#/components/schemas/Dog'
       - $ref: '#/components/schemas/Cat'
      discriminator:
        propertyName: petType
        mapping:
          dog: '#/components/schemas/Dog'
          cat: '#/components/schemas/Cat'
    Dog:
      properties:
        breed:
          type: string
        barkVolume:
          type: integer
          format: int32
          description: The volume of the dog's bark
    Cat:
      properties:
        color:
          type: string
        isIndoor:
          type: boolean
          description: Indicates if the cat is an indoor cat

This generates:

class Dog(BaseModel):
    breed: Optional[str] = None
    barkVolume: Optional[int] = Field(None, description="The volume of the dog's bark")
    petType: Literal['dog']

What I would like is petType: Literal['dog'] = 'dog'.

But I think the ordering of those actions prevents that

@koxudaxi
Copy link
Owner

@prmshepherd
Thank you for explaining it
Could you please add the unittest for the case?


class RequestV1(RequestBase):
request_id: str = Field(..., description='there is description', title='test title')
version: Literal['v1'] = 'v1'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generator now correctly adds the default for the literal

'openapi',
'--output-model-type',
'pydantic_v2.BaseModel',
'--use-one-literal-as-default',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting --use-one-literal-as-default in the test

@prmshepherd prmshepherd marked this pull request as ready for review September 13, 2024 09:34
@prmshepherd
Copy link
Contributor Author

@koxudaxi Sorry for the delay on this. I've added the test case now

@prmshepherd
Copy link
Contributor Author

prmshepherd commented Sep 17, 2024

@koxudaxi I've fixed that test error with the extraneous args - I'm not sure why I didn't see that on my local run.

@prmshepherd
Copy link
Contributor Author

@koxudaxi is anything more needed from me for this one?

@koxudaxi koxudaxi merged commit 295b758 into koxudaxi:main Sep 26, 2024
76 checks passed
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.

2 participants