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: enable path parsing to account for root level paths #1638

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

Conversation

LukeHagar
Copy link

@LukeHagar LukeHagar commented Jul 18, 2024

What/Why/How?

Fixes #1634

Reference

Testing

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@LukeHagar LukeHagar requested a review from a team as a code owner July 18, 2024 07:41
Copy link

changeset-bot bot commented Jul 18, 2024

🦋 Changeset detected

Latest commit: 3003f7f

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

This PR includes changesets to release 2 packages
Name Type
@redocly/cli Patch
@redocly/openapi-core 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

@lornajane
Copy link
Contributor

Thanks for the pull request @LukeHagar , we'll take a look.

@tatomyr
Copy link
Contributor

tatomyr commented Jul 22, 2024

@LukeHagar do you mind adding a simple e2e test for the case?
(The corresponding folder is __tests__/split).

@LukeHagar
Copy link
Author

I'd be happy too, I realize this solution would also collide with any API endpoints that are actually /root so I'll solve for that as well.

@tatomyr tatomyr marked this pull request as draft August 22, 2024 07:46
@LukeHagar LukeHagar marked this pull request as ready for review October 3, 2024 02:03
@LukeHagar
Copy link
Author

@lornajane @tatomyr I think this is right. I couldn't get any of the tests to run locally due to a warning about an outdated dep messing with the snapshot output

@tatomyr
Copy link
Contributor

tatomyr commented Oct 3, 2024

I couldn't get any of the tests to run locally due to a warning about an outdated dep messing with the snapshot output

It's most likely because you're using Node version 21 or higher. If you try version 20 or earlier, it should work locally.

@LukeHagar
Copy link
Author

Yup I'm on v22.8.0

@LukeHagar LukeHagar requested a review from a team as a code owner October 7, 2024 13:14
@@ -130,6 +130,9 @@ export function printExecutionTime(commandName: string, startedAt: number, api:
}

export function pathToFilename(path: string, pathSeparator: string) {
if (path === '/') {
return '~root';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that it will create a file named ~root for the root path?

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.

root level paths / are not split correctly
6 participants