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

Searching for csproj, fsproj that uses Microsoft.Net.Sdk.Web #10704

Merged

Conversation

issacnitin
Copy link
Contributor

No description provided.

@issacnitin issacnitin requested a review from sachinma June 20, 2019 10:17
@sachinma sachinma self-assigned this Jun 20, 2019
Tasks/DotNetCoreCLIV2/make.json Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
@sachinma
Copy link
Member

Add UTs for this change

@hiyadav
Copy link
Contributor

hiyadav commented Jun 21, 2019

Can you make sure that the assumption about "microsoft.net.sdk.web" is correct in case of fsproj as well.

@hiyadav
Copy link
Contributor

hiyadav commented Jun 24, 2019

Can you make sure that the assumption about "microsoft.net.sdk.web" is correct in case of fsproj as well.

Do verify this before you merge the PR. As when we devised this logic, we only thought and verified it for csproj files. Verification for fsproj and vbproj was never done.
Based on your verification, we will either only provide this for csproj or all (csproj, vbproj and fsproj)

@issacnitin
Copy link
Contributor Author

Can you make sure that the assumption about "microsoft.net.sdk.web" is correct in case of fsproj as well.

Do verify this before you merge the PR. As when we devised this logic, we only thought and verified it for csproj files. Verification for fsproj and vbproj was never done.
Based on your verification, we will either only provide this for csproj or all (csproj, vbproj and fsproj)

Yeah, this is verified for csproj and fsproj. For visual basic i think it's supported from .net core 3, will check before merging

@hiyadav
Copy link
Contributor

hiyadav commented Jun 24, 2019

Can you make sure that the assumption about "microsoft.net.sdk.web" is correct in case of fsproj as well.

Do verify this before you merge the PR. As when we devised this logic, we only thought and verified it for csproj files. Verification for fsproj and vbproj was never done.
Based on your verification, we will either only provide this for csproj or all (csproj, vbproj and fsproj)

Yeah, this is verified for csproj and fsproj. For visual basic i think it's supported from .net core 3, will check before merging

In case of vbProj, if it is only valid for .Net Core 3.0 and above, then we should not use this logic for vbproj. 3.0 will be RC in July and GA in September, till then its under preview.

Tasks/DotNetCoreCLIV2/dotnetcore.ts Outdated Show resolved Hide resolved
Tasks/DotNetCoreCLIV2/dotnetcore.ts Show resolved Hide resolved
@issacnitin issacnitin changed the title Searching for csproj, vbproj, fsproj that uses Microsoft.Net.Sdk.Web Searching for csproj, fsproj that uses Microsoft.Net.Sdk.Web Jun 25, 2019
@issacnitin issacnitin merged commit ef62d98 into master Jun 25, 2019
@@ -312,6 +312,19 @@ describe('DotNetCoreExe Suite', function () {
done();
});


it('publish works with publishWebProjects option if .csproj have Microsoft.Net.Sdk.Web', (done: MochaDone) => {
process.env["__projects__"] = "havesdk*/*.csproj;";
Copy link
Member

Choose a reason for hiding this comment

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

how is this test testing your new feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are mocking inputfile argument and defining a mapping for the argument, then we're mocking the output of that definition as well.

issacnitin added a commit that referenced this pull request Jun 27, 2019
* Searching for csproj, vbproj, fsproj using sdk Microsoft.Net.Sdk.Web

* Updating package.json

* Updating error message

* Review comments

* Review comments

* Try-catch

* Iteratively find and check for file encodings

* Review comments

* Supporting only utf encodings

* L0

* Adding semicolons

* Removing vbproj

* Comments

* Updating error message

* Updating error message
issacnitin added a commit that referenced this pull request Jun 27, 2019
* Searching for csproj, vbproj, fsproj using sdk Microsoft.Net.Sdk.Web

* Updating package.json

* Updating error message

* Review comments

* Review comments

* Try-catch

* Iteratively find and check for file encodings

* Review comments

* Supporting only utf encodings

* L0

* Adding semicolons

* Removing vbproj

* Comments

* Updating error message

* Updating error message
issacnitin pushed a commit that referenced this pull request Jun 30, 2019
….Sdk.Web (#10704) (#10775)

* Searching for csproj, fsproj that uses Microsoft.Net.Sdk.Web (#10704)

* Searching for csproj, vbproj, fsproj using sdk Microsoft.Net.Sdk.Web

* Updating package.json

* Updating error message

* Review comments

* Review comments

* Try-catch

* Iteratively find and check for file encodings

* Review comments

* Supporting only utf encodings

* L0

* Adding semicolons

* Removing vbproj

* Comments

* Updating error message

* Updating error message

* Updating L0 for DotNetCoreCLIV2 task (#10773)

* Updating L0

* Removing unnecessary imports

* Review comments
issacnitin pushed a commit that referenced this pull request Jul 9, 2019
…soft.Net.Sdk.Web (#10776)

* Searching for csproj, fsproj that uses Microsoft.Net.Sdk.Web (#10704)

* Searching for csproj, vbproj, fsproj using sdk Microsoft.Net.Sdk.Web

* Updating package.json

* Updating error message

* Review comments

* Review comments

* Try-catch

* Iteratively find and check for file encodings

* Review comments

* Supporting only utf encodings

* L0

* Adding semicolons

* Removing vbproj

* Comments

* Updating error message

* Updating error message

* Updating task version

* Updating L0 for DotNetCoreCLIV2 task (#10773)

* Updating L0

* Removing unnecessary imports

* Review comments
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