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 path check when folder name has spaces #836

Merged
merged 1 commit into from
Apr 12, 2020

Conversation

xt0rted
Copy link
Member

@xt0rted xt0rted commented Mar 8, 2020

While working on some more .net support I came across a file that wouldn't resolve. The folder it's in has a space in it and the captureQuotedWord regex excludes spaces so I removed that but it still wouldn't resolve.

After digging into it more I found that this check was returning false because the file paths in githubTree have spaces while the value used to compare against contains %20.

I'm not sure if this is the correct spot to fix this, but it's a good way to see the issue.

While running this version of the code go to this url and add/remove the replace fix.

https://github.com/microsoft/uprove-csharp-sdk/blob/master/UProveUnitTest/UProveUnitTest.csproj#L78

@stefanbuck
Copy link
Member

Sounds sensible to me! Do you mind adding an E2E test to cover the new behaviour?

@xt0rted
Copy link
Member Author

xt0rted commented Mar 9, 2020

Sure thing. When debugging this, is there a way to attach vscode or something so you can set breakpoints and step through the code? I tracked this down using a lot of console.info() calls and it wasn't fun.

@stefanbuck
Copy link
Member

Sorry for the late response, I was off sick.

I can imagine how painful debugging with console statements is. In JavaScript ther is keyword called debugger which pauses JavaScript execution when dev tools are open see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/debugger

@stefanbuck
Copy link
Member

or are you referring to E2E tests?

@xt0rted
Copy link
Member Author

xt0rted commented Mar 24, 2020

I was trying to debug in the browser with devtools open. I forgot about debugger, I'll give that a try later and try to get both of these finished up.

@xt0rted xt0rted marked this pull request as ready for review April 9, 2020 00:31
@xt0rted
Copy link
Member Author

xt0rted commented Apr 9, 2020

@stefanbuck I adjusted this so it only applies to the .net file paths, otherwise it was breaking other areas like JS imports.

@stefanbuck
Copy link
Member

I agree, it's best to limit this change to .net files to avoid side effects.

@stefanbuck stefanbuck merged commit 9ddbd77 into OctoLinker:master Apr 12, 2020
@xt0rted xt0rted deleted the patch-1 branch April 12, 2020 21:07
@xt0rted
Copy link
Member Author

xt0rted commented May 20, 2020

@stefanbuck I found that the extensions https://github.com/glebm/render-whitespace-on-github and https://github.com/sindresorhus/refined-github both break linking for files/folders with spaces in them due to showing the whitespace (which is an inserted <span> in each spot). These tend to run before Octolinker so our replacement can't find the string in the html. Think this is something we could try to work around?

This is what our test file looks like.

image

And the html with the visible whitespace.

<tr>
  <td id="L62" class="blob-num js-line-number" data-line-number="62"></td>
  <td id="LC62" class="blob-code blob-code-inner js-file-line rgh-observing-whitespace highlighted"><span data-rgh-whitespace="····">    </span>&lt;<span class="pl-ent">Content</span><span data-rgh-whitespace="·"> </span><span class="pl-e">Include</span>=<span class="pl-s"><span class="pl-pds">"</span>../foo<span data-rgh-whitespace="·"> </span>bar/test<span data-rgh-whitespace="·"> </span>file.cs<span class="pl-pds">"</span></span><span data-rgh-whitespace="·"> </span>/&gt;</td>
</tr>

@stefanbuck
Copy link
Member

For me this feels like an edge case. I rarely, almost never saw whitespaces in file and path names. Is this a .net thing?

@xt0rted
Copy link
Member Author

xt0rted commented May 27, 2020

I noticed it in some older projects I've been working on. I don't think it's super common, but it's definitely possible and I've seen it many times over the years. I'm fine leaving this as-is.

@stefanbuck
Copy link
Member

Do you mind creating an issue for this just in case we want to fix it at some point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants