Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add CodeGenerator tests and fix NREs/bugs - #40838

Merged
stephentoub merged 7 commits into
dotnet:masterfrom
hughbe:codedom-letsdothis
Nov 8, 2019
Merged

Add CodeGenerator tests and fix NREs/bugs#40838
stephentoub merged 7 commits into
dotnet:masterfrom
hughbe:codedom-letsdothis

Conversation

@hughbe

@hughbe hughbe commented Sep 5, 2019 •
edited
Loading

Copy link
Copy Markdown

Depends on #39585

@danmoseley
danmoseley requested a review from buyaa-n September 19, 2019 00:34
@danmoseley

danmoseley commented Sep 19, 2019

Copy link
Copy Markdown

@buyaa-n your area, could you please help get this in

krwq
krwq reviewed Sep 19, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs
@krwq

krwq commented Sep 19, 2019

Copy link
Copy Markdown
Member

product changes LGTM (with one comment)

@hughbe does this need rebase on top of that PR? Presumably some changes will disappear

@buyaa-n

buyaa-n commented Sep 19, 2019 •
edited
Loading

Copy link
Copy Markdown

Thank you @hughbe there is some test failures System.CodeDom.Tests Total: 3174, Errors: 0, Failed: 61, Skipped: 0, Time: 4. could take a look, excpet test failures looks great 👍. https://helix.dot.net/api/2019-06-17/jobs/8bd796a7-24f0-43b2-9566-be1cf5947d97/workitems/System.CodeDom.Tests/console

@maryamariyan

maryamariyan commented Oct 8, 2019

Copy link
Copy Markdown

Thanks for your PR @hughbe
Just checking if this is still under your radar and if you'll want to update it soon?
Assigned to you so it's easier for you to track.

stephentoub
stephentoub reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs Outdated
stephentoub
stephentoub reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs Outdated
stephentoub
stephentoub reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs Outdated
stephentoub
stephentoub reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs Outdated
stephentoub
stephentoub suggested changes Oct 23, 2019
View reviewed changes

@stephentoub stephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @hughbe.

I'm fine with the ArgumentNullException changes and the associated tests, but I think the changes that guard calls based on whether collections are empty should be reverted.

@buyaa-n

buyaa-n commented Oct 25, 2019

Copy link
Copy Markdown

I'm fine with the ArgumentNullException changes and the associated tests, but I think the changes that guard calls based on whether collections are empty should be reverted.

@hughbe if you have no time to apply the requested changes in near future please let me know, I can help wiith that. Thank you!

@maryamariyan
maryamariyan requested a review from krwq November 1, 2019 16:46
@danmoseley

danmoseley commented Nov 6, 2019

Copy link
Copy Markdown

@buyaa-n I suggest just making the changes so we can get this one merged...

@buyaa-n

buyaa-n commented Nov 6, 2019

Copy link
Copy Markdown

@buyaa-n I suggest just making the changes so we can get this one merged...

Sure

@buyaa-n

buyaa-n commented Nov 6, 2019

Copy link
Copy Markdown

/azp run corefx-ci

@azure-pipelines

azure-pipelines Bot commented Nov 6, 2019

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@buyaa-n

buyaa-n commented Nov 6, 2019

Copy link
Copy Markdown

I'm fine with the ArgumentNullException changes and the associated tests, but I think the changes that guard calls based on whether collections are empty should be reverted

@stephentoub reverted collection count checks, do you have any other comment?

@maryamariyan

maryamariyan commented Nov 6, 2019

Copy link
Copy Markdown

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your corefx repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

@hughbe

hughbe commented Nov 7, 2019

Copy link
Copy Markdown
Author

So sorry! I keep getting distracted by winforms stuff and university work. Thanks for taking this on.

@buyaa-n

buyaa-n commented Nov 7, 2019

Copy link
Copy Markdown

So sorry! I keep getting distracted by winforms stuff and university work. Thanks for taking this on.

Sure no problem, thank you for your contribution

krwq
krwq approved these changes Nov 7, 2019
View reviewed changes

@krwq krwq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Product changes and small tests file LGTM, haven't got time to look at the big tests change

stephentoub
stephentoub reviewed Nov 8, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs
if (member == null)
{
throw new ArgumentNullException(nameof(member));
}

@stephentoub stephentoub Nov 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

stephentoub
stephentoub reviewed Nov 8, 2019
View reviewed changes
Comment thread src/System.CodeDom/src/System/CodeDom/Compiler/CodeGenerator.cs
GenerateLinePragmaStart(imp.LinePragma);
}

GenerateNamespaceImport(imp);

@stephentoub stephentoub Nov 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
GenerateNamespaceImport(imp);
GenerateNamespaceImport(imp);

@stephentoub
stephentoub merged commit 7c867c2 into dotnet:master Nov 8, 2019
@hughbe
hughbe deleted the codedom-letsdothis branch November 8, 2019 17:28
@karelz karelz added this to the 5.0 milestone Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@buyaa-n buyaa-n Awaiting requested review from buyaa-n
2 more reviewers
@krwq krwq krwq approved these changes
@stephentoub stephentoub stephentoub approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@hughbe hughbe

Labels

area-System.CodeDom

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

7 participants

@hughbe @danmoseley @krwq @buyaa-n @maryamariyan @stephentoub @karelz