Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Fix issue with Binder on objects with indexers #272

Closed
wants to merge 2 commits into from
Closed

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Aug 12, 2015

Fixes #253

cc @kirthik @divega

{
var builder = new ConfigurationBuilder();
var config = builder.Build();
var options = ConfigurationBinder.Bind<List<string>>(config);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any asserts for this test. We should be verifying something right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to throw before the fix, so I don't typically use Assert.NotThrow... but that's basically what it this test is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert.NotNull?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that's fine

@kirthik
Copy link
Contributor

kirthik commented Aug 12, 2015

:shipit:

@@ -157,6 +157,14 @@ public void ConsistentExceptionOnFailedBinding(Type type)
}

[Fact]
public void BinderDoesNotChokeOnIndexer()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choke --> Throw please 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better, BinderIgnoresIndexerProperties.

@Eilon
Copy link
Member

Eilon commented Aug 13, 2015

But other than that, yeah, :shipit:

@HaoK
Copy link
Member Author

HaoK commented Aug 13, 2015

66df433

@HaoK HaoK closed this Aug 13, 2015
@natemcmaster natemcmaster deleted the haok/8-12fix branch June 12, 2017 21:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants