search

OpenXbox / xbox-smartglass-csharp

🎮.NET Standard client library for the Xbox One SmartGlass protocol
MIT License
67 stars 20 forks source link

Json Converter usage #96

Closed queequac closed 4 years ago

queequac commented 4 years ago

Issue #81 was closed very fast, but when reviewing the code from the associated PR (https://github.com/OpenXbox/xbox-smartglass-csharp/pull/82), I stumbled over some line where I'd have expected something different. (See my comment shown as review in the PR.)

Does that belong to Json Fragments and is that an uncompleted feature? It seems either the converter is not needed if the wrong type does not lead to any issues, or the converter is currently not called at all.🤔 Unsure how to test this, and I don't want to simply replace the type there in line 21.

tuxuser commented 4 years ago

@queequac Thanks for bringing that up again. @mitchcapper Can you comment on that?

mitchcapper commented 4 years ago

Sorry, fixed in the local code base but didn't submit a PR. I was curious as to why it was being called and functioning, however CanConvert is not always called for some of the explicit types we have (so as to not mess the test cases up). Changing it to the correct type does cause it to always be called at the right points, but as it isn't doing any custom conversions currently we can actually just rely on the base json resolver without issue.