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

Handle execution under root more correctly #154

Merged
merged 1 commit into from
Mar 3, 2020

Conversation

P0lip
Copy link
Member

@P0lip P0lip commented Feb 24, 2020

Right now, the file url starts with triple slashes (///). This PR fixes it.

@coveralls
Copy link

coveralls commented Feb 24, 2020

Pull Request Test Coverage Report for Build 388

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 93.908%

Totals Coverage Status
Change from base Build 381: 0.06%
Covered Lines: 679
Relevant Lines: 712

💛 - Coveralls

Copy link
Member

@JamesMessinger JamesMessinger 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 the PR @P0lip !

Can you provide an example of the problem that you're fixing? What does the URL/path that your getting look like currently, and what do you think it should look like?

lib/util/url.js Outdated Show resolved Hide resolved
process.cwd = processCwd; // already restored at line 19, but just in case
});

it("should resolve successfully a $ref pointing at absolute location", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

To match the other test suites, there need to be tests for the parse(), resolve(), dereference(), and bundle() methods, each with absolute paths, relative paths, and URLs. I recommend copying the contents of "test/specs/external" into this directory and using that as your starting point

Copy link
Member Author

Choose a reason for hiding this comment

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

relative paths

This is not going to work since I spoof process.cwd. I'd need to spoof fs calls as well.
Are you okay with that?

test/specs/absolute-root/absolute-root.spec.js Outdated Show resolved Hide resolved
@P0lip
Copy link
Member Author

P0lip commented Feb 26, 2020

Can you provide an example of the problem that you're fixing? What does the URL/path that your getting look like currently, and what do you think it should look like?

So, the fs library we use for browsers in case of Stoplight Studio, operates on / as its entry point, therefore process.cwd() equals /.

@JamesMessinger
Copy link
Member

Sorry for the delayed response. Last week got crazy busy.

So, the fs library we use for browsers in case of Stoplight Studio, operates on / as its entry point, therefore process.cwd() equals /.

Right, but what's the result of this? What's the problem that you're seeing and fixing? I want to make sure we're fixing the right problem in the right place. And that we have the right tests in place.

@JamesMessinger
Copy link
Member

@P0lip - can you please enable edits from maintainers on this PR?

@P0lip
Copy link
Member Author

P0lip commented Mar 2, 2020

Right, but what's the result of this? What's the problem that you're seeing and fixing? I want to make sure we're fixing the right problem in the right place. And that we have the right tests in place.

The result is that we try reading such paths starting with more than a single slash, i.e ///foo. Node's fs handles them fine, but not every library implementing fs-like interface does it, as the paths are not correct.

@P0lip - can you please enable edits from maintainers on this PR?

Hm, I cannot seem to find that option.
It's neither visible in the edit view nor in the sidebar 🤔

@JamesMessinger JamesMessinger merged commit 0dd90ca into APIDevTools:master Mar 3, 2020
@JamesMessinger
Copy link
Member

@P0lip - can you please enable edits from maintainers on this PR?

Hm, I cannot seem to find that option.
It's neither visible in the edit view nor in the sidebar 🤔

Huh... weird. I wonder if it's an org-level setting that someone at Stoplight would need to allow. 🤷‍♂️

Anyway... the PR looks good 👍 I have some changes to the tests, which I'll push after merging your changes.

@P0lip
Copy link
Member Author

P0lip commented Mar 5, 2020

Huh... weird. I wonder if it's an org-level setting that someone at Stoplight would need to allow. man_shrugging

Hm, that might be the case. I'll touch base with folks to see whether there is such an option exposed somewhere in the panel.

Anyway... the PR looks good +1 I have some changes to the tests, which I'll push after merging your changes.

Thanks!

@P0lip P0lip deleted the fix/root-execution branch March 18, 2024 10:08
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.

None yet

3 participants