Skip to content

Conversation

nkolev92
Copy link
Contributor

@nkolev92 nkolev92 commented Apr 27, 2020

Fixes #10947

Add support for reading the aliases flag from the assets file.

Note that this requires a NuGet insertion which has not happened yet.

cc @wli3

I want to port this eventually to a 3.1.x.

Copy link

@wli3 wli3 left a comment

Choose a reason for hiding this comment

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

Some questions. I think it is good in general. also + @dsplaisted in case you find something.

And the test will not pass until the insertion is in?

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

👍

@nkolev92 nkolev92 force-pushed the master-nkolev92-externAliases branch 2 times, most recently from 76bcbbe to 16de953 Compare April 28, 2020 23:40
@nkolev92 nkolev92 requested a review from wli3 April 29, 2020 01:08
@nkolev92
Copy link
Contributor Author

Now that the NuGet dependency has been merged, do I need to rebase to run the tests with the updated NuGet, or is kicking the build off again sufficient?

@dsplaisted
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsplaisted
Copy link
Member

I believe that kicking off the build again (which I've done) should be sufficient

@nkolev92
Copy link
Contributor Author

Failed with a compile error :(

@nkolev92 nkolev92 force-pushed the master-nkolev92-externAliases branch from 619425f to 9cd5885 Compare May 12, 2020 22:37
@nkolev92
Copy link
Contributor Author

Squashed my commits into 1 to retrigger the CI.

I'll rebase if it fails with a compile error again.

@nkolev92 nkolev92 force-pushed the master-nkolev92-externAliases branch from 9cd5885 to fcf4d75 Compare May 13, 2020 20:43
@nkolev92 nkolev92 force-pushed the master-nkolev92-externAliases branch from fcf4d75 to d06ac71 Compare May 21, 2020 05:16
@dsplaisted
Copy link
Member

@nkolev92 Is this ready to merge? It looks like tests are failing on .NET Framework only. If it's an issue where VS hasn't been updated with some functionality this feature needs, then you can use the RequiresMSBuildVersion Fact and Theory attributes to skip the test on full framework for now.

@nkolev92
Copy link
Contributor Author

I had to do some printf debugging to get that error message on the CI :)

Let me fix that now @dsplaisted!

Thanks for the pointer.

@nkolev92
Copy link
Contributor Author

Pushed again.

@dsplaisted
Copy link
Member

@nkolev92 If those tests are eventually supposed to work on Full MSBuild, it would be preferrable to use RequiresMSBuildVersionFacts.

@nkolev92 nkolev92 force-pushed the master-nkolev92-externAliases branch from 5cef742 to e82c23f Compare May 28, 2020 08:46
@nkolev92
Copy link
Contributor Author

@dsplaisted Fixed.

@nkolev92
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nkolev92
Copy link
Contributor Author

@dsplaisted

Should be good now.
Looking at the tests tab, they only ran for core.

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.

Handle the Aliases compile item metadata for PackageReference

3 participants