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

Improve PaddingConverter CreateInstance implementation #2679

Merged
merged 2 commits into from Jan 17, 2020

Conversation

@weltkante
Copy link
Contributor

weltkante commented Jan 9, 2020

Fixes #2677

Proposed changes

  • The context argument may be null for callers other than the PropertyGrid, this should not lead to an exception, in particular since GetCreateInstanceSupported returns true for a null context. Also every other implementation of CreateInstance I could find in WinForms and System.Drawing supports a null context, so PaddingConverter is an outlier.

Regression?

  • No

Risk

  • Callers which depend on an exception being thrown in a previously unsupported scenario will no longer see an exception

Before

  • PaddingConverter.CreateInstance(null, properties) throws an exception even though PaddingConverter.GetCreateInstanceSupported(null) returns true

After

  • PaddingConverter.CreateInstance(null, properties) creates an instance based on the given properties like any other TypeConverter defined in WinForms and System.Drawing

Test methodology

  • Tests have not been updated yet, this is WIP for discussion
Microsoft Reviewers: Open in CodeFlow
@weltkante weltkante requested a review from dotnet/dotnet-winforms as a code owner Jan 9, 2020
@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 10, 2020

I think this is a sensible proposal.
Looks like all other converters do the right thing and use context if it's not null. None of them throw.
Though it appears we have few more possible NREs related to use of context in MdiWindowListItemConverter and LanguageCultureInfoConverter.

On a side note, a number of UITypeEditors appear to like to live dangerously and use context without checks for null.

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 10, 2020

I'm curious, is this reachable at all?

            catch (NullReferenceException nullRef)
            {
                throw new ArgumentException(SR.PropertyValueInvalidEntry, nameof(propertyValues), nullRef);
            }
@weltkante

This comment has been minimized.

Copy link
Contributor Author

weltkante commented Jan 10, 2020

I'm curious, is this reachable at all?

You get this instead of a typecast exception when a value is null; also values are null for missing keys when the caller uses a Hashtable to store the properties.

@weltkante weltkante changed the title [WIP] Improve PaddingConverter CreateInstance implementation Improve PaddingConverter CreateInstance implementation Jan 17, 2020
@weltkante

This comment has been minimized.

Copy link
Contributor Author

weltkante commented Jan 17, 2020

Updated the tests. I think the existing tests (with updated expectations) already cover the modified code paths sufficiently.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #2679 into master will increase coverage by 0.563%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##              master       #2679         +/-   ##
===================================================
+ Coverage   57.59523%   58.15824%   +0.56301%     
===================================================
  Files           1235        1240          +5     
  Lines         418368      423008       +4640     
  Branches       38816       38852         +36     
===================================================
+ Hits          240960      246014       +5054     
+ Misses        172035      171620        -415     
- Partials        5373        5374          +1
Flag Coverage Δ
#Debug 58.15824% <100%> (+0.563%) ⬆️
#production 31.49243% <ø> (+0.17304%) ⬆️
#test 98.92984% <100%> (-0.00364%) ⬇️
@RussKie RussKie merged commit 8fe7c9b into dotnet:master Jan 17, 2020
5 checks passed
5 checks passed
WIP Ready for review
Details
dotnet-winforms CI Build #20200117.4 succeeded
Details
dotnet-winforms CI (Build Windows Debug) Build Windows Debug succeeded
Details
dotnet-winforms CI (Build Windows Release) Build Windows Release succeeded
Details
license/cla All CLA requirements met.
Details
@msftbot msftbot bot added this to the 5.0 milestone Jan 17, 2020
@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 17, 2020

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.