Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Use SourceLinkMap from Microsoft.SourceLink.Tools to implement SourceLink JSON parsing. #389

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmat
Copy link
Collaborator

@tmat tmat commented Oct 10, 2019

Creating and using a source package that includes the implementation is tracked by dotnet/sourcelink#443.
Fixes #386.

@tmat
Copy link
Collaborator Author

tmat commented Oct 10, 2019

@ctaggart Please take a look.

@tmat
Copy link
Collaborator Author

tmat commented Oct 10, 2019

/cc @dougbu

Copy link

@dougbu dougbu left a 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.
Copy link

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
Copy link

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:
Copy link

Choose a reason for hiding this comment

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

nit:

Suggested change
// trim BOM if present:
// trim UTF8 BOM if present:

Also: Doesn't Newtonsoft.Json handle a UTF8 BOM? @JamesNK?

Copy link

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

@ctaggart
Copy link
Owner

The azure-pipelines failure looks like a build environment issue:

D:\a\1\s\dotnet-sourcelink-shared\SourceLinkMap.cs(117,20): error CS8107: Feature 'default literal' is not available in C# 7.0. Please use language version 7.1 or greater. [D:\a\1\s\dotnet-sourcelink\sourcelink.csproj]

Looks good. I'll probably just stick to AppVeyor to get this out.

@ctaggart ctaggart added this to the 3.2.0 milestone Oct 11, 2019
@ctaggart
Copy link
Owner

I can push a 3.2.0 here next week when you are ready.
https://www.nuget.org/packages/sourcelink/

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)
Copy link
Owner

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 ?

Copy link
Collaborator Author

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.

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

Successfully merging this pull request may close these issues.

sourcelink test does not match URLs correctly
4 participants