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

Skip Schema prefix trimmin if --enum is used with --root-types-no-schema-prefix #2206

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sultaniman
Copy link
Contributor

@sultaniman sultaniman commented Mar 17, 2025

Changes

This PR is a follow up to #1958 and as a result disables --root-types-no-schema-prefix when --enum flag passed.

More on why this is necessary: #1958 (comment)

How to Review

Check out the mentioned PR and review this one.

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

@sultaniman sultaniman requested a review from a team as a code owner March 17, 2025 20:15
@sultaniman sultaniman requested a review from gzm0 March 17, 2025 20:15
Copy link

netlify bot commented Mar 17, 2025

👷 Deploy request for openapi-ts pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 3392361

Copy link

changeset-bot bot commented Mar 17, 2025

⚠️ No Changeset found

Latest commit: 3392361

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@sultaniman sultaniman force-pushed the sultaniman/fix/skip-schema-trim-when-enum branch from ba8e5ef to 31054dc Compare March 17, 2025 20:18
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Nice! Some comments about behavior / comments. The code itself looks great!

@sultaniman
Copy link
Contributor Author

sultaniman commented Mar 18, 2025

Thanks for taking a look at this, I will address your comments.

@sultaniman sultaniman requested a review from fluctus March 19, 2025 20:37
@sultaniman sultaniman force-pushed the sultaniman/fix/skip-schema-trim-when-enum branch 2 times, most recently from 91394c2 to cf6d0a1 Compare March 19, 2025 20:38
@sultaniman sultaniman force-pushed the sultaniman/fix/skip-schema-trim-when-enum branch from cf6d0a1 to 3392361 Compare March 19, 2025 20:39
@sultaniman sultaniman requested a review from gzm0 March 19, 2025 20:39
Copy link

@fluctus fluctus left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to fix this @sultaniman!
Please see my suggestion

Comment on lines +84 to +88
// Skipping --root-types generation only for enums if --enum is set
// while still applying --root-types-no-schema-prefix to other types.
if (!(ctx.enum && item.enum !== undefined)) {
aliasName = aliasName.replace(componentKey, "");
}
Copy link

@fluctus fluctus Mar 30, 2025

Choose a reason for hiding this comment

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

@sultaniman While this fixes the issue, it introduces an inconsistency in the generated types. Everything else will have the Schema prefix removed, but for enums, we’ll end up with both EnumName (the enum itself) and an unnecessary SchemaEnumName.

Since EnumName already serves as both a value and a TypeScript type due to the nature of enums, SchemaEnumName seems redundant. Wouldn't it make more sense to omit the root type for enums entirely when --root-types-no-schema-prefix is used? (That was my suggestion in the original PR.)

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.

3 participants