Skip to content

Don't throw in logging when the document path contains curly braces (#61524)#61535

Merged
dibarbet merged 1 commit intodotnet:release/dev17.2from
dibarbet:port_logging_172
May 31, 2022
Merged

Don't throw in logging when the document path contains curly braces (#61524)#61535
dibarbet merged 1 commit intodotnet:release/dev17.2from
dibarbet:port_logging_172

Conversation

@dibarbet
Copy link
Member

Port of #61524 to 17.2

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1514768

Needs to be backported because this affects Razor - @NTaylorMullen FYI

@dibarbet dibarbet added Area-IDE LSP issues related to the roslyn language server protocol implementation labels May 26, 2022
@dibarbet dibarbet requested a review from a team as a code owner May 26, 2022 17:58
Copy link

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Is there other areas that are potentially at risk here? Also, is there a way to ban the _traceSource.TraceInformation API from LSP based projects?

@dibarbet
Copy link
Member Author

Is there other areas that are potentially at risk here?

All our LSP logging code goes through here, so that is covered. I think other issues are probably unlikely- all the roslyn code already handles these documents. And I didn't encounter issues during testing with the Razor LSP server either (the logs show the correct file paths), only when it calls out to roslyn

Also, is there a way to ban the _traceSource.TraceInformation API from LSP based projects?

We can definitely do this. I can send a PR in main to add that

@dibarbet dibarbet changed the base branch from main to release/dev17.2 May 26, 2022 19:46
@dibarbet dibarbet merged commit 8d3180e into dotnet:release/dev17.2 May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE LSP issues related to the roslyn language server protocol implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants