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.

Sign up for GitHub

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

Jump to bottom

Refactor CLI path handling, fixing several bugs #1281

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Refactor CLI path handling, fixing several bugs #1281

merged 1 commit into from
Aug 9, 2023

Conversation

pvanagtmaal
Copy link
Contributor

Changes

Fixes #1091

Uses of URL have been replaced by their path variants. This makes more sense because we're handling actual file paths here. This fixes several bugs, including the reporting one, but also for example exporting to absolute paths.

How to Review

Checklist

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

@changeset-bot
Copy link

changeset-bot bot commented Aug 8, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 4b20b25

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-typescript Patch

Not sure what this means? Click here to learn what changesets are.

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

@drwpow
Copy link
Contributor

drwpow commented Aug 8, 2023 •
edited
Loading

I would prefer to always work with URLs as they’re much more resilient to preventing Windows vs Unix path bugs (fileURLToPath), easy to modify, and prevent sloppy string mashing. They actually are meant for files, not just remote resources (that’s the whole design of the file: protocol). Though we don’t have a ton of Windows tests in this repo, in other applications URLs have saved me hundreds of hours of debugging time from my work on Snowpack & Astro, and other projects like SvelteKit take this approach. I believe this bug could be fixed without undoing the URLs.

Edit: for clarity, I think keeping partial paths as strings makes sense! But for absolute paths, like CWD, I’d like to keep those as URLs whenever possible.

improve CLI output path handling
4b20b25 
Fixes:
- output directories without trailing slash
- output directories with globs
- output directories when the converted yaml is more than one level deeper than cwd (e.g. `./bin/cli.js 'test/fixtures/*.yaml' --output foo`)
@pvanagtmaal pvanagtmaal mentioned this pull request Aug 9, 2023
Closed
2 tasks
drwpow
drwpow approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

Thank you! Awesome fix

@drwpow drwpow merged commit ebd31ff into openapi-ts:main Aug 9, 2023
5 checks passed
@github-actions github-actions bot mentioned this pull request Aug 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drwpow drwpow drwpow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bulk types generation
2 participants
@pvanagtmaal @drwpow