Skip to content

Fixing availability of ShowCellToolTips and ShowCellErrors setters #1853

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
Oct 30, 2019
Merged

Fixing availability of ShowCellToolTips and ShowCellErrors setters #1853

merged 1 commit into from
Oct 30, 2019

Conversation

@vladimir-krestov
Copy link
Contributor

@vladimir-krestov vladimir-krestov commented Sep 10, 2019 •
edited by ghost
Loading

Proposed changes

  • Change implementation of the set blocks of ShowCellToolTips and ShowCellErrors properties in DataGridView.cs to avoid possible problems

❗️ We must not use getters in setters.
We already had a serious bug (#1661) due to such use.

Customer Impact

  • No

Regression?

  • No

Risk

  • No

Test environment(s)

  • Microsoft Windows [Version 10.0.18362.295]
  • .Net Core version: 3.0.0-rc1-19455-02
Microsoft Reviewers: Open in CodeFlow

@vladimir-krestov vladimir-krestov added this to the 3.1 milestone Sep 10, 2019
@vladimir-krestov vladimir-krestov requested a review from a team as a code owner September 10, 2019 14:46
@vladimir-krestov vladimir-krestov self-assigned this Sep 10, 2019
RussKie
RussKie previously approved these changes Sep 10, 2019
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.

👍

@vladimir-krestov vladimir-krestov added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 10, 2019
@RussKie RussKie changed the title Fixing availability of a setter in a getter WIP: Fixing availability of a setter in a getter Sep 10, 2019
@vladimir-krestov vladimir-krestov force-pushed the dev/v-vlkres/GetterInSetter branch from e221c26 to 224f7bd Compare September 10, 2019 16:18
@vladimir-krestov vladimir-krestov changed the base branch from release/3.1 to master September 10, 2019 16:18
@vladimir-krestov vladimir-krestov removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 10, 2019
@vladimir-krestov vladimir-krestov removed this from the 3.1 milestone Sep 10, 2019
@vladimir-krestov vladimir-krestov changed the title WIP: Fixing availability of a setter in a getter Fixing availability of a setter in a getter Sep 10, 2019
@RussKie RussKie changed the title Fixing availability of a setter in a getter WIP: Fixing availability of a setter in a getter Sep 11, 2019
@zsd4yr zsd4yr changed the title WIP: Fixing availability of a setter in a getter WIP: Fixing availability of ShowCellToolTips and ShowCellErrors setters Sep 11, 2019
@codecov
Copy link

codecov bot commented Oct 14, 2019 •
edited
Loading

Codecov Report

Merging #1853 into master will decrease coverage by 0.053%.
The diff coverage is 0%.

@@                Coverage Diff                @@
##              master       #1853       +/-   ##
=================================================
- Coverage   29.27774%   29.22474%   -0.053%     
=================================================
  Files            940         940               
  Lines         266677      266685        +8     
  Branches       37947       37947               
=================================================
- Hits           78077       77938      -139     
- Misses        183373      183529      +156     
+ Partials        5227        5218        -9
Flag Coverage Δ
#Debug 29.22474% <0%> (-0.05301%) ⬇️
#production 29.22474% <0%> (-0.05301%) ⬇️
#test 100% <ø> (ø) ⬆️

@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 14, 2019

When some developers will change the code of these properties, they will not care about the implementation of the getters in the setters. We must take preventive measures to avoid bugs.

@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 14, 2019

Needs code review.

@vladimir-krestov vladimir-krestov changed the title WIP: Fixing availability of ShowCellToolTips and ShowCellErrors setters Fixing availability of ShowCellToolTips and ShowCellErrors setters Oct 14, 2019
RussKie
RussKie previously approved these changes Oct 14, 2019
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.

LGTM, though the cyclomatic complexity concerns me greatly...

@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 14, 2019

In my opinion, it is minimal nesting.

@merriemcgaw
Copy link
Member

merriemcgaw commented Oct 15, 2019

Approved on my end. Do we need testing before adding the Ask Mode label. @RussKie

@merriemcgaw merriemcgaw added this to the 3.1 milestone Oct 15, 2019
@RussKie RussKie modified the milestones: 3.1, 5.0 Oct 15, 2019
@RussKie
Copy link
Contributor

RussKie commented Oct 15, 2019

This change is targeting 5.0, so no "ask-mode" :)

@vladimir-krestov vladimir-krestov added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 16, 2019
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 21, 2019

Testing is blocked now for the master branch.

@vladimir-krestov vladimir-krestov removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 21, 2019
@zsd4yr
Copy link
Contributor

zsd4yr commented Oct 21, 2019

Testing is blocked now for the master branch.

Why is that?

@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 22, 2019

At the moment testers can't test this fix using Core 5.0.
@Vino-Wang could you please tell us why is that?

@Vino-Wang
Copy link

Vino-Wang commented Oct 23, 2019

@vladimir-krestov, here is the issue #2156

@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 23, 2019

Thank you.

@vladimir-krestov vladimir-krestov changed the title Fixing availability of ShowCellToolTips and ShowCellErrors setters WIP: Fixing availability of ShowCellToolTips and ShowCellErrors setters Oct 24, 2019
@vladimir-krestov vladimir-krestov added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 24, 2019
@vladimir-krestov
Copy link
Contributor Author

vladimir-krestov commented Oct 29, 2019 •
edited
Loading

Testers approved these changes. ✔️

  • Run all Core RT cases  -- passed, no regression issue
  • Inspect for the DataGridView control -- no any changes
  • Narrator -- have the same results with .NET Core 5.0

@vladimir-krestov vladimir-krestov changed the title WIP: Fixing availability of ShowCellToolTips and ShowCellErrors setters Fixing availability of ShowCellToolTips and ShowCellErrors setters Oct 29, 2019
@vladimir-krestov vladimir-krestov added waiting-review This item is waiting on review by one or more members of team and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Oct 29, 2019
RussKie
RussKie previously approved these changes Oct 29, 2019
View reviewed changes
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Oct 29, 2019
@vladimir-krestov vladimir-krestov force-pushed the dev/v-vlkres/GetterInSetter branch from 670ff40 to 8db67da Compare October 29, 2019 11:56
@RussKie RussKie merged commit 905b81b into master Oct 30, 2019
@RussKie RussKie deleted the dev/v-vlkres/GetterInSetter branch October 30, 2019 00:02
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@zsd4yr zsd4yr zsd4yr left review comments

@RussKie RussKie RussKie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vladimir-krestov vladimir-krestov

Labels

None yet

Projects

None yet

Milestone

5.0 Previews 1-4

Development

Successfully merging this pull request may close these issues.

6 participants

@vladimir-krestov @merriemcgaw @RussKie @zsd4yr @Vino-Wang