Skip to content
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

Add JSON RPC #10

Merged
merged 1 commit into from
Aug 18, 2015
Merged

Add JSON RPC #10

merged 1 commit into from
Aug 18, 2015

Conversation

daniel-dressler
Copy link
Contributor

This class depends upon HTTPRequest.

Note that the unity sdk does not support web sockets yet.


// Basic authentication
private string _username;
private string _password;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That the privates are all underscore prefixed does not match how other classes handle this. Any objections to marking the underscores for removal?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of using underscores for private/protected in languages that don't need it (like C#) to begin with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nullorvoid has a standard on this, not sure what it got changed to

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still in debate for the style guide. The single main reason for this is the use of properties. For Zappallas we are trying lowercase private and public with Uppercase properties. This is going OK .. but as expected when you have

public string type;
public string Type { get { return type; } set { OnTypeSet.Invoke(); type = value; }

or even

public void SetType(string type) { OnTypeSet.Invoke(); this.type = type; }

We have already had cases of people using the public string type to set the parameter. So it relies heavily on code review (This is my single biggest grip with Unity still)

I'm almost swaying to the _ notation just to separate public properties that are public from the setters / getters / functions that handle the set (not that you should have these in C# either)

Copy link
Collaborator

Choose a reason for hiding this comment

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

will be addressed by #14

@AlmirKadric
Copy link
Collaborator

let's merge this, i'll address all issues in a separate PR

ronkorving pushed a commit that referenced this pull request Aug 18, 2015
@ronkorving ronkorving merged commit 1bf1c29 into mage:develop Aug 18, 2015
@AlmirKadric AlmirKadric deleted the feature/jsonrpc branch August 18, 2015 07:13
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.

4 participants