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: allow descriptions on enum refs #473

Conversation

JoshuaKGoldberg
Copy link
Contributor

Fixes #472.

return definition.enum === schema.enum
}
return definition === schema
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sold that this is a particularly good solution 🙃 it feels weird to hardcode the case for enums. But I'm not familiar enough with the codebase to have any deeper insights.

Copy link

@dhvector dhvector Aug 1, 2022

Choose a reason for hiding this comment

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

I noticed the same on #466, but not related to enums. In their case, they had a property like this:

"definitions: {
  "status_block": {
    "type": "object",
    "properties": {
      "experimental": {
        "type": "boolean"
      }
    }
  }
},
"status": {
  "$ref": "#/definitions/status_block",
  "description": "..."
}

See my comment for more details on why this happens.

For a more universal fix that goes beyond just enums, I wonder about:

  1. create a copy of the original json, set aside for step 4.
  2. cull certain fields from any subschemas that use $ref
  3. (Now the $ref resolver can make the $ref referentially equal to what it's referencing, so that json-schema-to-typescript doesn't generate a new type (or inline, depending on what version you're using?). This won't work if there are fields not culled in step 2, of course)
  4. During code generation, reference the original json for things like description, which shouldn't affect type relationships, but should certainly affect generated doc strings for properties.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, although that sounds reasonable, I'm very new to this project and haven't done a deep dive into it. So if you say that's a good fix then I believe you! 😄

Comment on lines +983 to +990
/**␊
* A first property.␊
*/␊
export type Shared = "a" | "b";␊
export interface Enum {␊
first?: Shared;␊
}␊
Copy link
Contributor

@bradzacher bradzacher Apr 12, 2023

Choose a reason for hiding this comment

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

The property comment is in the wrong place

-   /**
-    * A first property.
-    */
    export type Shared = "a" | "b";
    
    export interface Enum {
+     /**
+      * A first property.
+      */
      first?: Shared;␊
    }␊

@bcherny
Copy link
Owner

bcherny commented May 4, 2023

Closing out stale PRs. Feel free to rebase and re-open if you'd like to revisit this PR.

@bcherny bcherny closed this May 4, 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.

Adding a description to a $ref object referencing an enum inlines the enum
4 participants