Skip to content

Move sub-classes of "UpDownBase", "WebBrowser" and "WebBrowserBase" classes to their own files #4579

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 22, 2021
Merged

Move sub-classes of "UpDownBase", "WebBrowser" and "WebBrowserBase" classes to their own files #4579

merged 1 commit into from
Feb 22, 2021

Conversation

SergeySmirnov-Akvelon
Copy link
Contributor

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon commented Feb 17, 2021 •
edited by ghost
Loading

Proposed changes

  • Move sub-classes of "UpDownBase", "WebBrowser" and "WebBrowserBase" classes to their own files

Customer Impact

  • No

Regression?

  • No

Risk

  • Minimal

Test environment(s)

  • Microsoft Windows [Version 10.0.19041.388]
  • .NET Core SDK 6.0.0-alpha.1.21073.5
Microsoft Reviewers: Open in CodeFlow

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon requested a review from a team as a code owner February 17, 2021 13:49
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon mentioned this pull request Feb 17, 2021
Closed
74 tasks
@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-4571_UpDownBase_WebBrowser_WebBrowserBase branch from 2ec2db0 to d453abc Compare February 17, 2021 14:12
@codecov
Copy link

codecov bot commented Feb 17, 2021 •
edited
Loading

Codecov Report

Merging #4579 (55ccd11) into main (8ee92e7) will increase coverage by 0.00001%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##                main       #4579         +/-   ##
===================================================
+ Coverage   97.96538%   97.96539%   +0.00001%     
===================================================
  Files            540         540                 
  Lines         263145      263146          +1     
  Branches        4923        4923                 
===================================================
+ Hits          257791      257792          +1     
  Misses          4478        4478                 
  Partials         876         876
Flag Coverage Δ
Debug 97.96539% <ø> (+0.00001%) ⬆️
test 97.96539% <ø> (+0.00001%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

RussKie
RussKie reviewed Feb 17, 2021
View reviewed changes
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍

dreddy-work
dreddy-work reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@dreddy-work dreddy-work left a comment

Choose a reason for hiding this comment

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

I know this is mostly moving around the code but its better to clean the parts of the code we are touching. Added some nits and suggestions we are following in the designer repo.

...ows.Forms/src/System/Windows/Forms/UpDownBase.UpDownButtons.UpDownButtonsAccessibleObject.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

are these missing naming convention?

...ows.Forms/src/System/Windows/Forms/UpDownBase.UpDownButtons.UpDownButtonsAccessibleObject.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

nit: we are encouraging condition expressions in other repos we own. Lets do that in this repo as well, at least in the code we are refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

We did few passes over parts of the codebase in the past, we should do another sweep. This is separate piece of work though.

Copy link
Member

@dreddy-work dreddy-work Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand that. But that separate work will keep postponed or never make to priority list. In the designer repo, we make it norm that if we touch the code, we clean it as much as possible. In this case, i do not see cleaning would be risky or time taking. Happy to chat offline.

...ows.Forms/src/System/Windows/Forms/UpDownBase.UpDownButtons.UpDownButtonsAccessibleObject.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

nit: conditional expression? and in other places in this PR.

src/System.Windows.Forms/src/System/Windows/Forms/WebBrowser.WebBrowserEvent.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

targetFrameName can not be empty here?

src/System.Windows.Forms/src/System/Windows/Forms/WebBrowser.WebBrowserEvent.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

should we throw if parent is null?

Copy link
Contributor

Choose a reason for hiding this comment

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

This falls into the NRTA initiative. Personally I'm inclined to do it separately for ease of tracking, and to avoid slowing down cleanups like this one.

src/System.Windows.Forms/src/System/Windows/Forms/WebBrowser.WebBrowserEvent.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

seems private class, is this hiding base class method?

src/System.Windows.Forms/src/System/Windows/Forms/WebBrowser.WebBrowserSite.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

lets not abbreviate declarations. In other repos, we are enforcing to give meaningful/readable names.

src/System.Windows.Forms/src/System/Windows/Forms/WebBrowser.WebBrowserSite.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

nit: expressional bodies here and below.

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon force-pushed the Issue-4571_UpDownBase_WebBrowser_WebBrowserBase branch from d453abc to 55ccd11 Compare February 18, 2021 06:57
@SergeySmirnov-Akvelon
Copy link
Contributor Author

Hi @dreddy-work. In this PR we are simply moving the sub-classes into separate files to keep the changes as simple and safe as possible. I suggest creating a separate ticket for refactoring.

@dreddy-work
Copy link
Member

dreddy-work commented Feb 19, 2021 •
edited
Loading

Hi @dreddy-work. In this PR we are simply moving the sub-classes into separate files to keep the changes as simple and safe as possible. I suggest creating a separate ticket for refactoring.

I understand that. Approach we follow in designer repo is, if we are touching the code base, we clean it up. in this case, it shouldn't be hard. I see this is going into main. So, risk is less.

@RussKie RussKie merged commit a6e8118 into dotnet:main Feb 22, 2021
@RussKie RussKie deleted the Issue-4571_UpDownBase_WebBrowser_WebBrowserBase branch February 22, 2021 09:30
@ghost ghost added this to the 6.0 Preview2 milestone Feb 22, 2021
@RussKie RussKie added the code cleanup cleanup code for unused apis/properties/comments - no functional changes. label Mar 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@RussKie RussKie RussKie left review comments

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

Reviewers whose approvals may not affect merge requirements
Assignees

@SergeySmirnov-Akvelon SergeySmirnov-Akvelon

Labels
code cleanup cleanup code for unused apis/properties/comments - no functional changes.
Projects
None yet
Milestone
6.0 Preview2
Development

Successfully merging this pull request may close these issues.
3 participants
@SergeySmirnov-Akvelon @dreddy-work @RussKie