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 constructors to SignalRMessage and SignalRGroupAction #53

Merged
merged 2 commits into from Mar 4, 2019

Conversation

@anthonychu
Copy link
Member

@anthonychu anthonychu commented Mar 4, 2019

Make it easier to instantiate these objects properly.

@anthonychu anthonychu requested a review from JialinXin Mar 4, 2019
* @param target Target method to invoke on clients
* @param arguments Arguments to pass to target method
*/
public SignalRMessage(String target, Object... arguments) {
Copy link
Contributor

@JialinXin JialinXin Mar 4, 2019

Choose a reason for hiding this comment

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

public SignalRMessage( [](start = 4, length = 22)

Shall we also add one contractor with userId that for send to user and one contractor with groupname that for send to group? #Resolved

Loading

Copy link
Contributor

@JialinXin JialinXin Mar 4, 2019

Choose a reason for hiding this comment

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

That should be handle in function. Ignore this.


In reply to: 261928906 [](ancestors = 261928906)

Loading

Copy link
Member Author

@anthonychu anthonychu Mar 4, 2019

Choose a reason for hiding this comment

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

Been thinking about this one. Because they are both String, I'm not sure how we can differentiate. Potentially, we can remove userId and groupName from SignalRMessage and create subclasses SignalRGroupMessage and SignalRUserMessage that have those as fields and in the constructors. This is probably a better way to do it, but might be too late to make this big of a change. #Resolved

Loading

Copy link
Contributor

@JialinXin JialinXin Mar 4, 2019

Choose a reason for hiding this comment

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

Yes. I'm thinking if it should be handled in app level or function level. It's not block issue. Let's consider it in next releasing cycle.


In reply to: 261930864 [](ancestors = 261930864)

Loading

Copy link
Contributor

@JialinXin JialinXin left a comment

🕐

Loading

Copy link
Contributor

@JialinXin JialinXin left a comment

:shipit:

Loading

*/
public SignalRMessage(String target, Object... arguments) {
this.target = target;
this.arguments.addAll(Arrays.asList(arguments));
Copy link
Contributor

@JialinXin JialinXin Mar 4, 2019

Choose a reason for hiding this comment

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

👍 That's what I'm looking for!

Loading

@JialinXin JialinXin merged commit df05118 into Azure:dev Mar 4, 2019
2 checks passed
Loading
wanlwanl added a commit that referenced this issue Jun 11, 2019
* ignore global.json to unblock local build (#52)

* add java scm for release required

* Add constructors to SignalRMessage and SignalRGroupAction (#53)

* Add constructors

* Change to use .addAll

* Add Azure Java Function simple-chat sample (#49)

* add java simple-chat sample

* Update version info

* update CORS to enable visit test site from file

* Fix sample function serialize issue.

* update extension reference to GA version

* test use (will revert before check-in)

* remove reference SNAPSHOT

* allow telemetry

* update function with class constructor changes

* use real plugin repositories

* Wanl/use mng sdk (#57)

* Update to the correct version for the extension (#60)

* add user's claims to azure signalr access token (#61)

* README.md Update - Removed the groups point under Current Limitations (#55)

* Removed the groups point under Current Limitation in README.md

* Update support scenario and limitation in README

* Exposing servicehubcontext for improving user experience  (#62)

* update version (#64)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants