search

CarterCommunity / Carter

Carter is framework that is a thin layer of extension methods and functionality over ASP.NET Core allowing code to be more explicit and most importantly more enjoyable.
MIT License
2.08k stars 175 forks source link

Fix exception with form binding and nullable types #199

Closed welshdave closed 5 years ago

welshdave commented 5 years ago

Fixes #198

jchannon commented 5 years ago

Looking good. Will take a proper look tomorrow. Thanks!

jchannon commented 5 years ago

This is up to you but I see a few comments here about some TypeConverters not working in certain circumstances and would like to expand the tests a bit to ensure that we can convert those. If you want to take a look let me know or I'll merge then take a look and fix any issues if they exist https://www.hanselman.com/blog/TypeConvertersTheresNotEnoughTypeDescripterGetConverterInTheWorld.aspx

welshdave commented 5 years ago

Wouldn't be able to look for a few days (travelling in to office to prove I still exist), but could look Thursday or Friday?

jchannon commented 5 years ago

Cool!

On Mon, 15 Jul 2019 at 09:24, Dave Lewis notifications@github.com wrote:

Wouldn't be able to look for a few days (travelling in to office to prove I still exist), but could look Thursday or Friday?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CarterCommunity/Carter/pull/199?email_source=notifications&email_token=AAAZVJRCGZJHJHEYOWAV2M3P7QX23A5CNFSM4IDRZZXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ5AM3Y#issuecomment-511313519, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAZVJSO7EZMTG5II4GBAZDP7QX23ANCNFSM4IDRZZXA .

welshdave commented 5 years ago

So TypeConverters have a few problems - don't think it's worth using them. I've updated and added a few more tests.

jchannon commented 5 years ago

What was the issues with them?

welshdave commented 5 years ago

They lose precision when converting datetime (loses milliseconds), seem to be a lot more picky about date formatting than other conversion methods.

And for number formatting, strings like "1,234.00" don't convert (try converting that to a decimal, it fails with the TypeConverter approach, works fine with Convert.ChangeType).

jchannon commented 5 years ago

Is it worth adding some tests for decimals?

welshdave commented 5 years ago

Yeah, I'll add some now

jchannon commented 5 years ago

Thanks @welshdave Awesome stuff!