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

Convert TestPlatform.vsix from V2 to V3 format. #316

Merged
merged 8 commits into from
Jan 5, 2017
Merged

Convert TestPlatform.vsix from V2 to V3 format. #316

merged 8 commits into from
Jan 5, 2017

Conversation

Faizan2304
Copy link
Contributor

Due to limitation with dotnet core tooling, we are going through zip approach.

IEnumerable<string> files = Directory.EnumerateFiles(inputDirectory, "*.*", SearchOption.AllDirectories);
int inputDirectoryLength = inputDirectory.Length;

using (ZipArchive vsixFile = ZipFile.Open(vsixFilePath, ZipArchiveMode.Create))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: always better to use var instead.

@@ -19,4 +20,7 @@
<Assets>
<Asset Type="Microsoft.TestPlatform.V2" Path="vstest.console.exe" />
</Assets>
</PackageManifest>
<Prerequisites>
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain this dependency please.
And should it be "Visual Studio Core Editor".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment here that it is by default?

Copy link
Contributor

@singhsarab singhsarab left a comment

Choose a reason for hiding this comment

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

👍

@@ -263,11 +263,17 @@ function Create-VsixPackage

Write-Log "Create-VsixPackage: Started."
$packageDir = Get-FullCLRPackageDirectory
$tpSrcDir = Join-Path $env:TP_ROOT_DIR "src"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion : remove whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next commit.


# Copy vsix manifests
$vsixManifests = @("*Content_Types*.xml",
"extension.vsixmanifest",
"License.rtf",
"catalog.json",
Copy link
Contributor

@harshjain2 harshjain2 Jan 5, 2017

Choose a reason for hiding this comment

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

Suggestion : update alignment for this and the line below.

Copy link
Contributor

@harshjain2 harshjain2 Jan 5, 2017

Choose a reason for hiding this comment

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

Instead of writing custom steps to copy these items, can we add these files as content to package.csproj so that they could get copied while publishing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Fix in next commit

ZipFile.CreateFromDirectory(inputDirectory, vsixFilePath, CompressionLevel.Fastest, false);
}
// Get all files to put in vsix
IEnumerable<string> files = Directory.EnumerateFiles(inputDirectory, "*.*", SearchOption.AllDirectories);
Copy link
Contributor

Choose a reason for hiding this comment

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

use var instead of "IEnumerable" as per resharper stylecop conventions.

@Faizan2304
Copy link
Contributor Author

@dotnet-bot test this please

@Faizan2304 Faizan2304 merged commit 8c179a7 into microsoft:master Jan 5, 2017
@Faizan2304 Faizan2304 deleted the v2tov3 branch January 5, 2017 20:46
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.

5 participants