-
Notifications
You must be signed in to change notification settings - Fork 55
Use SourceLinkMap from Microsoft.SourceLink.Tools to implement SourceLink JSON parsing. #389
base: master
Are you sure you want to change the base?
Conversation
… Link JSON parsing. Creating and using a source package that includes the [implementation](https://github.com/dotnet/symreader-converter/blob/master/src/Microsoft.DiaSymReader.Converter/SourceLinkMap.cs) is tracked by dotnet/sourcelink#443. Fixes ctaggart#386.
@ctaggart Please take a look. |
/cc @dougbu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions…
@@ -0,0 +1,198 @@ | |||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this header correct in this repo?
|
||
// TODO: Use a source package once available (https://github.com/dotnet/sourcelink/issues/443) | ||
|
||
namespace Microsoft.SourceLink.Tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should this file include a comment pointing toward the source in the dotnet/sourcelink repo?
var list = new List<(FilePathPattern key, UriPattern value)>(); | ||
try | ||
{ | ||
// trim BOM if present: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
// trim BOM if present: | |
// trim UTF8 BOM if present: |
Also: Doesn't Newtonsoft.Json handle a UTF8 BOM? @JamesNK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses StreamReader, so yes
The azure-pipelines failure looks like a build environment issue:
Looks good. I'll probably just stick to AppVeyor to get this out. |
I can push a 3.2.0 here next week when you are ready. It would be great if you were to replace this global test tool with one from dotnet/sourcelink in the future. |
using Newtonsoft.Json; | ||
using Newtonsoft.Json.Linq; | ||
|
||
// TODO: Use a source package once available (https://github.com/dotnet/sourcelink/issues/443) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be waiting on this dotnet/sourcelink#443 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to wait for that work item to be done. I just haven't had time to finish this PR. Feel free to take it over.
Creating and using a source package that includes the implementation is tracked by dotnet/sourcelink#443.
Fixes #386.