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

refactor: only accept string in pathToFsPath #3812

Merged
merged 2 commits into from
Jul 21, 2021
Merged

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Jul 19, 2021

This PR reverts some work done in #3751 to make pathToFsPath better.

@jsjoeio jsjoeio self-assigned this Jul 19, 2021
@jsjoeio jsjoeio added this to the 3.11.0 milestone Jul 19, 2021
@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #3812 (48f28e3) into main (0f45152) will increase coverage by 0.05%.
The diff coverage is 20.00%.

❗ Current head 48f28e3 differs from pull request most recent head 6e33dcc. Consider uploading reports for the commit 6e33dcc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3812      +/-   ##
==========================================
+ Coverage   62.05%   62.10%   +0.05%     
==========================================
  Files          36       36              
  Lines        1863     1863              
  Branches      378      378              
==========================================
+ Hits         1156     1157       +1     
+ Misses        601      600       -1     
  Partials      106      106              
Impacted Files Coverage Δ
src/node/routes/vscode.ts 29.47% <0.00%> (-0.64%) ⬇️
src/node/util.ts 81.53% <100.00%> (+1.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f45152...6e33dcc. Read the comment docs.

@jsjoeio jsjoeio marked this pull request as ready for review July 19, 2021 22:02
@jsjoeio jsjoeio requested a review from a team as a code owner July 19, 2021 22:02
@jsjoeio jsjoeio modified the milestones: 3.11.0, 3.12.0 Jul 19, 2021
Copy link
Contributor

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

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

LGTM! Codecov shamin' us a little for lack of test coverage here though 😂

const isWindows = process.platform === "win32"
const uri = { authority: undefined, path: getFirstString(path), scheme: "file" }
const uri = { authority: undefined, path: getFirstString(path) || "", scheme: "file" }
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering - would this be more readable in expanded form, e.g.

const uri = {
  authority: undefined,
  path: getFirstString(path) || "",
  scheme: "file",
}

I'm not sure if prettier prefers this form though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely is for me! I think Prettier autoformats it though :(

it("should not throw an error for a string array", () => {
// @ts-expect-error We need to check other types
expect(() => util.pathToFsPath(["/hello/foo", "/hello/bar"]).not.toThrow())
})
it("should use the first string in a string array", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no more string array, right? so the description is now outdated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll update that

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jul 20, 2021

LGTM! Codecov shamin' us a little for lack of test coverage here though 😂

ugh I know 😂 I think it's cause I touched some of the routes files, but I didn't write any tests for them so it's unhappy. I'm going to say it's okay for now since at least I'm increasing coverage.

CodeQL caught a path where we were passing in req.query.path
to pathToFsPath, which may not have been a string.

So we refactored some things to ensure we only pass it a string
which also let us change the parameter type to string
instead of string | string[].
@jsjoeio jsjoeio merged commit 670f0a1 into main Jul 21, 2021
@jsjoeio jsjoeio deleted the jsjoeio-fix-pathToFsPath branch July 21, 2021 19:06
@jsjoeio jsjoeio modified the milestones: 3.12.0, 3.11.1 Aug 6, 2021
@ahmadyahya11 ahmadyahya11 mentioned this pull request Aug 8, 2021
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.

2 participants