fix Conversions setting to allow a local overide to specify Conversion.Default when the Global value is set to somthing else#813
Conversation
|
Thanks very much for the PR - great work. I'll check back in a few hours to take a further look. Thanks again! |
|
|
||
| public static bool IsValidFlags(this Conversions value) => (int) value >= 0 && (int) value < _maxConversion; | ||
|
|
||
| public static bool IsValidFlags(this Conversions value) => (int) value >= -1 && (int) value < _maxConversion; |
There was a problem hiding this comment.
I think this should remain as >=0 as this check is done after config is merged. Nothing should remain unspecified after merging as that's when the defaults are applied. An unspecified value might mean we've forgotten to handle the default.
There was a problem hiding this comment.
I changed this as without my change I was getting hundreds of VOG011 diagnostic warnings from the tests.
Is this the wrong place to fix that?
There was a problem hiding this comment.
Oh, I see. When I get chance, I'll pull down this branch and take a look.
There was a problem hiding this comment.
Apologies for the delay, I hope to have a bit of time at the weekend to catch up on things. Appreciate your PR!
There was a problem hiding this comment.
So sorry for the extremely long delay! I'm on it now!
There was a problem hiding this comment.
I checked out this PR and pushed, but for some reason, it created a new PR
#871
|
Closing this as it has now been merged in PR #871 |
2 participants
Fixes #810
Adds a new "Unspecifed" member to the Conversions Enum and uses that rather than Default
Adds unit tests for the new behavour and the previously broken behavour.
I have kept the "Conversions.Default" member for backwards compatibility - as any change to that causes a huge chunk of test fails. However I think it could be removed in the next major version.