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

fix(core): don't union required with already required props #1878

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

AllieJonsson
Copy link
Contributor

Status

READY

Description

Fixes #1877

Previously, /generated/default/all-of/getNotHasPropertiesWithAllOfPets200.ts had this generated code:

export type GetNotHasPropertiesWithAllOfPets200 = Pet &
  PetDetail &
  GetNotHasPropertiesWithAllOfPets200AllOf &
  GetNotHasPropertiesWithAllOfPets200AllOfTwo &
  Required<
    Pick<
      Pet &
        PetDetail &
        GetNotHasPropertiesWithAllOfPets200AllOf &
        GetNotHasPropertiesWithAllOfPets200AllOfTwo,
      'category' | 'color'
    >
  >;

The Required<Pick<..., 'category' | 'color'>> is unnecessary in this case, as both 'category' and 'color' is already required in GetNotHasPropertiesWithAllOfPets200AllOf and GetNotHasPropertiesWithAllOfPets200AllOfTwo.

This PR fixes that and only creates this Required<Pick< when there are non-required properties in a child object that should be required in this top level object. The generated code in /generated/default/all-of/getNotHasPropertiesWithAllOfPets200.ts is now:

export type GetNotHasPropertiesWithAllOfPets200 = Pet &
  PetDetail &
  GetNotHasPropertiesWithAllOfPets200AllOf &
  GetNotHasPropertiesWithAllOfPets200AllOfTwo;

Related PRs

List related PRs against other branches:

branch PR
require-inside-all-of #1819

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

@AllieJonsson
Copy link
Contributor Author

Is it possible to re-run tests? It seems like there was a network error when installing dependencies :(

@melloware
Copy link
Collaborator

I just kicked them off again

Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

@AllieJonsson
I can check before and after the improvement, so could you let me know the test?. Or add it if it does not exist.

@AllieJonsson
Copy link
Contributor Author

@AllieJonsson I can check before and after the improvement, so could you let me know the test?. Or add it if it does not exist.
It is all-of.yaml

@melloware melloware requested a review from soartec-lab February 6, 2025 15:25
Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

It's great! thanks!

@soartec-lab soartec-lab merged commit 3d122c3 into orval-labs:master Feb 7, 2025
2 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.

Excessive code is generated for allof with required properties
3 participants