Skip to content
This repository has been archived by the owner on Apr 27, 2019. It is now read-only.

deserialization name of node #23

Merged
merged 1 commit into from
Oct 30, 2015
Merged

Conversation

lockwobr
Copy link
Contributor

the name on node does not deserialize, added name property so it
deserializes the name of the node

the name on node does not deserialize, added name property so it
deserializes the name of the node
@highlyunavailable
Copy link
Contributor

Thanks for the PR. I'm traveling right now but I'll look at it as soon as I can.

@highlyunavailable
Copy link
Contributor

@lockwobr Thanks for this, good catch. I think I did it because Node.Node was confusing or it wouldn't compile? Maybe I was outsmarting myself here when I ported this section of code. Technically this is a deviation from the Go Consul API and this should be named the same as that since I am trying to match the C# and Go ones as closely as possible to avoid making it difficult to make changes when the upstream changes. I'll merge this because it is a problem but the name really should be Node. II'll change it in Develop, so in the next version this will be a breaking change that will require renaming references to this property in your code, but that will drop when Consul 0.6.0 comes out. Thanks again, though.

highlyunavailable added a commit that referenced this pull request Oct 30, 2015
deserialization name of node
@highlyunavailable highlyunavailable merged commit 8c20970 into PlayFab:master Oct 30, 2015
@highlyunavailable
Copy link
Contributor

Oh yeah, that was the problem:

Catalog.cs(27,23): error CS0542: 'Node': member names cannot be the same as their enclosing type

So yeah, this will have to stay as "Name".

@lockwobr
Copy link
Contributor Author

I feel like there might be others like this that are not getting deserialized correctly. The way that JSON.net works is it has to match the JSON exactly. I am not 100% sure of this, it was just a feeling I got from trying to use it, but I was only in need of nodes (name and address) info so I stopped looking at it. Something someone could do is update the tests to check for fields to not be null/empty.

@highlyunavailable
Copy link
Contributor

Could you please file an issue so I remember to add a test for these?

@highlyunavailable
Copy link
Contributor

@lockwobr As I remember, Node was the only one that was affected this way but this is a very good idea (to check deserialization against deserialized data during tests) so I've made #24 to track this.

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

2 participants