Skip to content

Fix/response ❤️ #2383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Mpdreamz merged 4 commits into from
Nov 21, 2016
Merged

Fix/response ❤️ #2383

Mpdreamz merged 4 commits into from
Nov 21, 2016

Conversation

@Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Nov 9, 2016

Give all the responses some ❤️

As per #2299 be more consistent responses with returning empty collections vs null on responses. We did this sporadically but not consistently.
Now that we dropped .NET 4.0 we can make the response IEnumerable and Dictionaries IReadOnly*

Made sure all the interfaces only define getters and not setters

gmarz
gmarz reviewed Nov 9, 2016
View reviewed changes
Copy link
Contributor

@gmarz gmarz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall LGTM 👍 I wonder if we should be returning concrete types (ReadOnlyDictionary, ReadOnlyCollection) rather than interfaces though.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Nov 9, 2016

Can you explain why you wonder that @gmarz? Interested to hear your thoughts on this.

@gmarz
Copy link
Contributor

gmarz commented Nov 9, 2016

@Mpdreamz Robustness principle http://stackoverflow.com/questions/17170/when-to-use-ilist-and-when-to-use-list

On the other hand, when returning an object out of a function, you want to give the user the richest possible set of operations without them having to cast around.

Previously the richest types would be List<T> and Dictionary<T>, and now with this PR ReadOnlyCollection<T> and ReadOnlyDictionary<T>.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Nov 9, 2016

Not sure how I feel about returning the concrete types since they expose a lot of operations we dont want to expose e.g searchResponse.Documents.Add(). I generally agree with Postel's law but I feel the concrete types set wrong expectations. Also it ties us down.

Need to sleep on this :)

@gmarz gmarz force-pushed the fix/response-love branch from 39bdf77 to d3e4345 Compare November 16, 2016 18:32
@Mpdreamz
Copy link
Member Author

Mpdreamz commented Nov 20, 2016

@gmarz @russcam rebased against master and fixed the flakey tests.

Still some integration test failures but they are unrelated to this branch.

@russcam
Copy link
Contributor

russcam commented Nov 20, 2016

Will have a look at this today @Mpdreamz

@russcam
Copy link
Contributor

russcam commented Nov 21, 2016

7 failing integration tests:

  • Tests.XPack.Security.User.PutUser.PutUserRunAsApiTests.ReturnsExpectedIsValid [FAIL]
  • Tests.XPack.Security.User.PutUser.PutUserRunAsApiTests.HandlesStatusCode [FAIL]
  • Tests.XPack.Security.Role.PutRole.PutRoleRunAsApiTests.HandlesStatusCode [FAIL]
  • Tests.XPack.Security.Role.PutRole.PutRoleRunAsApiTests.ReturnsExpectedIsValid [FAIL]

related to the param for shield run as #2394

  • Tests.Indices.StatusManagement.Upgrade.UpgradeApiTests.ReturnsExpectedIsValid [FAIL]
  • Tests.Indices.StatusManagement.Upgrade.UpgradeApiTests.HandlesStatusCode [FAIL]

related to allow_no_indices #2397

  • Tests.Indices.Monitoring.IndicesShardStores.IndicesShardStoresApiTests.AssertResponse [FAIL]

not sure why this one is failing right now. Expected collection not to be empty.

@russcam
Copy link
Contributor

russcam commented Nov 21, 2016

LGTM - would be good to have some Coding Standards unit tests to ensure that this is the case across all IResponse types

@Mpdreamz
Give all the responses some ❤️
6d57a0d 
As per #2299 be more consistent responses with returning empty collections vs null on responses.
Now that we dropped .NET 4.0 we can make the response IEnumerable and Dictionaries IReadOnly*

Made sure all the interfaces only define getters and not setters
@Mpdreamz
another pass at all response objects
c9b331b
@Mpdreamz
Make sure EmptyReadOnly returns actual ReadOnly* instances for the in…
8dd61ad 
…terfaces and not rely on implicit conversion.

Fix sorts on covariant hits having trouble going from JArray to ReadOnlyCollection<object>
@Mpdreamz
fix shard store response because Stores now needs an explicit JsonPro…
d4d2af8 
…perty
@Mpdreamz
Copy link
Member Author

Mpdreamz commented Nov 21, 2016

The IndicesShardResponse was an easy fix, the property now needed an explicit [JsonProperty], squashing and merging this one in.

@Mpdreamz Mpdreamz merged commit 30ac823 into master Nov 21, 2016
Mpdreamz added a commit that referenced this pull request Nov 21, 2016
@Mpdreamz
Fix/response ❤️ (#2383)
5d68e20 
* Give all the responses some ❤️

As per #2299 be more consistent responses with returning empty collections vs null on responses.
Now that we dropped .NET 4.0 we can make the response IEnumerable and Dictionaries IReadOnly*

Made sure all the interfaces only define getters and not setters

* another pass at all response objects

* Make sure EmptyReadOnly returns actual ReadOnly* instances for the interfaces and not rely on implicit conversion.
Fix sorts on covariant hits having trouble going from JArray to ReadOnlyCollection<object>

* fix shard store response because Stores now needs an explicit JsonProperty

Conflicts:
	src/Tests/tests.yaml
@Mpdreamz Mpdreamz mentioned this pull request Nov 21, 2016
Closed
@Mpdreamz Mpdreamz deleted the fix/response-love branch November 30, 2016 16:58
awelburn pushed a commit to Artesian/elasticsearch-net that referenced this pull request Nov 6, 2017
@Mpdreamz
Fix/response ❤️ (elastic#2383)
7b50192 
* Give all the responses some ❤️

As per elastic#2299 be more consistent responses with returning empty collections vs null on responses.
Now that we dropped .NET 4.0 we can make the response IEnumerable and Dictionaries IReadOnly*

Made sure all the interfaces only define getters and not setters

* another pass at all response objects

* Make sure EmptyReadOnly returns actual ReadOnly* instances for the interfaces and not rely on implicit conversion.
Fix sorts on covariant hits having trouble going from JArray to ReadOnlyCollection<object>

* fix shard store response because Stores now needs an explicit JsonProperty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gmarz gmarz gmarz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gmarz gmarz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Mpdreamz @gmarz @russcam