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

feat(cli): Add inline property to type references from properties in Fern definition with OpenAPI importer #5248

Merged
merged 26 commits into from
Nov 27, 2024

Conversation

Swimburger
Copy link
Member

@Swimburger Swimburger commented Nov 21, 2024

Description

In the Fern definition, you can express your desire to inline a type from a property on a type or request:

service:
  endpoints:
    endpointName:
      ...
      request:
        ...
        body:
          ...
          properties:
            bar:
              type: InlineType

types:
  RootType:
    properties:
      bar:
        type: InlineType
  InlineType:
    inline: true
    properties:
      ...

The IR generated from this Fern definition will store the desire to inline a type on the type declaration.

Changes Made

  • Update Fern definition to allow inline true on declarations
  • Update OpenAPI parser and OpenAPI IR to pass inline to the generated Fern definition
  • Update Fern to IR generator to pass on inline to types

Testing

  • Unit tests added/updated

Added more test definitions + evaluate existing test definition snapshots.

Copy link

github-actions bot commented Nov 21, 2024


BaseTypeDeclarationSchema:
extends:
inline: optional<boolean>
Copy link
Member

Choose a reason for hiding this comment

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

i assume this is the only thing that changed? nit: would be nice to remove the formatting changes for this PR

@@ -283,6 +283,7 @@ types:
type: string
docs: |
A unique name for the property.
inline: optional<boolean>
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this property? If a SchemaWithExample contains another SchemaWithExample that is not a reference, you already know the type is inlined

Copy link
Member

Choose a reason for hiding this comment

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

In other words, in openapi-ir-to-fern when you call buildTypeDeclaration() and then that calls buildTypeReference and that calls buildTypeDeclaration again, you know that the second declaration call is for an inlined type.

Copy link
Member

Choose a reason for hiding this comment

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

So if we just tracked depth of call, then i think we should be able to control inline based on that

Copy link
Contributor

@amckinney amckinney left a comment

Choose a reason for hiding this comment

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

Looks good to me - only a couple comments worth considering before merging!

AliasSchema:
### Aliases ####

AliasSchema:
extends: BaseTypeDeclarationSchema
Copy link
Contributor

Choose a reason for hiding this comment

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

Should WithInline just be part of BaseTypeDeclarationSchema? It looks like we omitted it from AliasSchema which is reasonable, but it's theoretically possible that we generate an alias with a bad name too, and we could just as easily create an in-lined alias type.

That way any TypeDeclaration moving forward will always be able to be in-lined. Curious to hear your thoughts though - I realize this case is less important overall.

Comment on lines +656 to +658
function getInline(declarationDepth: number): boolean | undefined {
return declarationDepth > 0 ? true : undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth leaving a function-level comment on this one - I could see us forgetting the rationale around how we're detecting whether or not a type is in-lined. It looks like this was inspired by this comment?

@Swimburger Swimburger enabled auto-merge (squash) November 27, 2024 20:15
@Swimburger Swimburger merged commit 9770c58 into main Nov 27, 2024
50 of 54 checks passed
@Swimburger Swimburger deleted the niels/cli/inline-types-phase1 branch November 27, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants