Skip to content

Port System.ComponentModel.Design.* for #469#606

Merged
JuditRose merged 10 commits intomasterfrom
WFI-592-port-system-componentmodel-design-for-469
Mar 22, 2019
Merged

Port System.ComponentModel.Design.* for #469#606
JuditRose merged 10 commits intomasterfrom
WFI-592-port-system-componentmodel-design-for-469

Conversation

@JuditRose
Copy link
Contributor

@JuditRose JuditRose commented Mar 18, 2019

Fixes #592
Fixes #469
Contributes to #221

Components are ported from framework.

@JuditRose JuditRose requested a review from a team as a code owner March 18, 2019 21:55
@JuditRose JuditRose mentioned this pull request Mar 18, 2019
Closed
5 tasks
zsd4yr
zsd4yr approved these changes Mar 19, 2019
View reviewed changes
Copy link
Contributor

@zsd4yr zsd4yr left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/InheritanceService.cs
}

// Now check the attributes of the field and get out if it isn't something that can be inherited.
Debug.Assert(!field.IsStatic, "Instance binding shouldn't have found this field");
Copy link
Contributor

@zsd4yr zsd4yr Mar 18, 2019

Choose a reason for hiding this comment

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

This should be either a Debug.WriteLineIf if it should not fail the process or a Debug.Fail if it should.

Copy link
Contributor

@zsd4yr zsd4yr Mar 18, 2019

Choose a reason for hiding this comment

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

Same for all the other Debug Asserts

Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik Mar 19, 2019

Choose a reason for hiding this comment

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

Hi @zsd4yr, could you please clarify what the problem with Asserts is? Breaking unit tests?

Copy link
Contributor Author

@JuditRose JuditRose Mar 20, 2019

Choose a reason for hiding this comment

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

It's not breaking the tests

Tanya-Solyanik
Tanya-Solyanik reviewed Mar 19, 2019
View reviewed changes
Tanya-Solyanik
Tanya-Solyanik suggested changes Mar 19, 2019
View reviewed changes
Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

Looks good, I flagged several classes of "clean-up" type issues, Please review the whole PR for these classes of issues, It will be much easier to run a reg expression search in the VS than eye-balling the code.

dreddy-work
dreddy-work reviewed Mar 19, 2019
View reviewed changes
@codecov
Copy link

codecov bot commented Mar 20, 2019 •
edited
Loading

Codecov Report

Merging #606 into master will decrease coverage by 0.21%.
The diff coverage is 3.47%.

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
- Coverage   27.89%   27.68%   -0.22%     
==========================================
  Files         907      926      +19     
  Lines      244382   247910    +3528     
  Branches    32225    32774     +549     
==========================================
+ Hits        68175    68631     +456     
- Misses     172379   175428    +3049     
- Partials     3828     3851      +23
Flag Coverage Δ
#Debug 27.68% <3.47%> (-0.22%) ⬇️
#production 19.32% <1.15%> (-0.14%) ⬇️
#test 98.71% <96.51%> (-0.02%) ⬇️

@JuditRose JuditRose merged commit e70fe16 into master Mar 22, 2019
@JuditRose JuditRose deleted the WFI-592-port-system-componentmodel-design-for-469 branch March 22, 2019 23:18
@JuditRose JuditRose mentioned this pull request Apr 9, 2019
Open
27 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@weltkante weltkante weltkante left review comments

@dreddy-work dreddy-work dreddy-work left review comments

@zsd4yr zsd4yr zsd4yr approved these changes

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JuditRose @weltkante @zsd4yr @Tanya-Solyanik @dreddy-work