Skip to content

Made ParseClient.ConvertTo() a public API. #71

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.

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

Merged
merged 1 commit into from
Jan 8, 2016

Conversation

richardjrossiii
Copy link
Contributor

ConvertTo's functionality is useful for external developers if they ever have to deal with CloudCode, or coercing other types return from the SDK.

Fixes #67.

@hallucinogen
Copy link
Contributor

I'm totally OK with this and this is something that currently only exists in .NET SDK. Question for the other team members:

  1. What do you think about opening this up?
  2. Does it make sense to implement this in other SDKs?
    @grantland @nlutsenko @wangmengyan95 @andrewimm

@nlutsenko
Copy link

iOS/OSX/watchOS/tvOS - simply type casting works great.
Don't feel strongly, but can see that this is valuable.
The only thing that comes to my mind - does it make sense to leave on ParseClient or rather it would be better to move it to something like Parse on .NET`?

@grantland
Copy link

Don't we already convert known datatypes? https://github.com/ParsePlatform/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ParseCloudCodeController.java#L45

We don't really allow deserializing from user JSON since odd things can happen, so I'm really wary of this.

Lastly, +1 to it oddly being on ParseClient

@wangmengyan95
Copy link

Don't feel strongly too. +1 for move it in to Parse. I want to know what odd things can happen when we deserializing from user JSON, any ideas? @richardjrossiii @nlutsenko @hallucinogen

@grantland
Copy link

What's the problem we're solving here? ParseCloud should be returning decoded responses.

@richardjrossiii
Copy link
Contributor Author

@grantland We're solving an issue with .NET's generic type system. When we decode from the server, we give back a dictionary of type Dictionary<string, object>. In .NET's type system, we do not have any way to treat that as a dictionary of type Dictionary<string, ParseObject>, as the two are separate types at run-time (generics are not type-erased as in java).

This becomes more useful as you get a deeper nested structure, as at each level you'll be dealing with either Dictionary<string, object> or List<Object>, and if you wish to grab any of those collections as another type, You're pretty much SOL.

Also, we don't have a Parse class on .NET. If we did, there would be a naming conflict between the Parse class and the Parse namespace, which is why we use ParseClient class.

@richardjrossiii richardjrossiii force-pushed the richardross.convertto.public branch from 73c9d86 to 880c5c3 Compare December 9, 2015 22:59
@wangmengyan95
Copy link

@richardjrossiii, thanks for the clarification. I am OK with this change on .NET. But probably we do not need this in Android.

ConvertTo's functionality is useful for external developers if they ever have to deal with CloudCode, or coercing other types return from the SDK.

Fixes #67.
@richardjrossiii richardjrossiii force-pushed the richardross.convertto.public branch from 880c5c3 to 5f0c45a Compare December 12, 2015 01:04
@richardjrossiii
Copy link
Contributor Author

Hey guys, I just updated the PR to move the utility into the Parse.Utilities namespace, under the invocation Conversion.As<T>(value). Let me know your thoughts.

/// <summary>
/// A set of utilities for converting generic types between each other.
/// </summary>
public static class Conversion {
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot the end result of our discussion. Do we want this to be public or just want to separate this out and mark as "pending decision whether we want to make it public/make it separate .dll or do nothing and leave it here"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want it public for now, if we decide to do it as a separate project/repository, we'll make it a dependency, and it would stay under the same namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

@hallucinogen
Copy link
Contributor

I like the direction we're going here. Several cleanup and one major question.

@richardjrossiii
Copy link
Contributor Author

I think I addressed all of your comments in various replies, but I forgot to pass it back. Whoops.

richardjrossiii added a commit that referenced this pull request Jan 8, 2016
@richardjrossiii richardjrossiii merged commit b801a6e into master Jan 8, 2016
@richardjrossiii richardjrossiii deleted the richardross.convertto.public branch January 8, 2016 01:43
@richardjrossiii richardjrossiii added this to the 1.7.0 milestone Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants