search

ChrisMissal / Formo

Formo allows you to use your configuration file as a dynamic object. Turn your web.config or application settings into a rich, dynamic object.
http://chrismissal.com/Formo/
MIT License
259 stars 33 forks source link

When ThrowIfNull is true, an exception is now thrown only if both the key with the classname and the key w/o it are missing from the config file #44

Closed urig closed 7 years ago

urig commented 7 years ago

Hi @ChrisMissal ,

This PR resolves issue https://github.com/ChrisMissal/Formo/issues/43 .
I recommend ignoring whitespace using ?w=1 because VS felt line endings were inconsistent in one of the files :) Please hold no punches when reviewing - Would love to learn and improve my PR-fu.

Thanks, Uri

urig commented 7 years ago

Hi @ChrisMissal, I'm sure you're very busy and I don't mean to be pushy but - ping? :)

ChrisMissal commented 7 years ago

Thanks for the reminder! I'm checking it out now.

ChrisMissal commented 7 years ago

@urig It feels like we're using Exceptions as control flow in the new GetValueOrNull method. Can we tackle this another way? I'm not saying I won't merge, but I wanted to see if you had other thoughts when working on this?

urig commented 7 years ago

Many thanks for the feedback Chris.

I've just taken some time to look at GetValueOrNull(). To avoid relying on exception throwing for control flow, I thought it would be straightforward to replace the call to 'configuration.ConnectionString.Get(key)' with a call to a "TryGet" method.

But it seems to me that this is going to be quite complicated as the only (?) option is to call ConnectionStringsConfiguration.TryGetMember(). That method's first parameter is of the abstract type GetMemberBinder and, to be frank, I don't know of a simple way of getting from string key to an instance of that. (This SO answer suggests that the way to invoke TryGetMember() itself relies on catching an exception)

If you know of a simpler approach, I'd love to learn and implement it. If not, then IMHO the current implementation is the lesser evil. :)

ChrisMissal commented 7 years ago

That's kind of where I landed as well, I just wanted to see what your thoughts were. The only final (tiny) thing I think I'd like is moving those new InvalidOperationExceptions into the ThrowHelper class so the message isn't repeated.

urig commented 7 years ago

Done. :)

ChrisMissal commented 7 years ago

Sorry @urig, life is beautiful chaos :)

I'll try to get a new NuGet out later tonight!

urig commented 7 years ago

Many thanks - much appreciated. Take your time, there's no rush :)

ChrisMissal commented 7 years ago

New NuGet package is released @urig - https://www.nuget.org/packages/Formo

urig commented 7 years ago

Thanks Chris, you're the best :)