FP NUnit1001: CustomTypeConverters could convert from anything #559
Conversation
| @@ -210,6 +210,10 @@ private static bool HasBuiltInImplicitConversion(ITypeSymbol argumentType, IType | |||
| } | |||
| } | |||
| } | |||
| else if (targetTypeSymbol.GetAttributes().Any(att => att.AttributeClass?.GetFullMetadataName() == typeof(TypeConverterAttribute).FullName)) | |||
There was a problem hiding this comment.
I'm not behind a computer, but isn't TypeConverter not already dealt with?
You added a vase when targetType is null?
The CustomTypeConverter in your test doesn't override the Can convert from tested in the existing code.
There was a problem hiding this comment.
TypeConverters are only dealt with for strings. And the code tries to check for types it can access the code from. In most cases that is not possible. So basically, you can not know if it is supported or not, but because of that, you should report on it. Specially because the default severity is an error, and it runs perfectly.
There was a problem hiding this comment.
Thanks. I am on leave without access to a development PC until middle of July. I will look further then.
There was a problem hiding this comment.
Sorry, I forgot to look at.
Your added case can be removed. Instead we can remove the condition on the line below.
manfred-brands
force-pushed
the
fp-type-converters-can-also-convert-from-other-types
branch
from
1bd4371
to
b59c277
Compare
| [TypeConverter(typeof(CustomTypeConverter))] | ||
| struct CustomType { } | ||
| class CustomTypeConverter : TypeConverter { }"); | ||
|
|
||
| RoslynAssert.Valid(this.analyzer, testCode); | ||
| } | ||
|
|
||
| [Test] | ||
| public void AnalyzeArgumentIsSomeConvertedToTypeWithCustomConverter() | ||
| { | ||
| var testCode = TestUtility.WrapClassInNamespaceAndAddUsing(@" | ||
| using System.ComponentModel; | ||
|
|
||
| class C | ||
| { | ||
| [TestCase(17)] | ||
| public void Test(CustomType p) { } | ||
| } | ||
|
|
There was a problem hiding this comment.
There is already a test for this case, but one that assumes failure.
| @@ -210,6 +210,10 @@ private static bool HasBuiltInImplicitConversion(ITypeSymbol argumentType, IType | |||
| } | |||
| } | |||
| } | |||
| else if (targetTypeSymbol.GetAttributes().Any(att => att.AttributeClass?.GetFullMetadataName() == typeof(TypeConverterAttribute).FullName)) | |||
There was a problem hiding this comment.
Sorry, I forgot to look at.
Your added case can be removed. Instead we can remove the condition on the line below.
|
@manfred-brands I saw you made the changes yourself. Would you like me to make any changes in this PR? |
No. Are you happy with the changes? |
Corniel
deleted the
fp-type-converters-can-also-convert-from-other-types
branch
I stumbled into this, as I use this feature quite often:
Where
Qowaiv.Financial.Amountis a value type that has its own type converter that not only can convert from string, but also fromint,long,double,float, anddecimal. This works like a charm, so it too bad that this falsely flags NUnit1001.I've provided a fix, But if the implementation should be changed, just let me no.
Note that none existing test started to fail with this changed behavior.