add control.update(), which takes a partial control #65

ProbablePrime · 2017-06-08T02:05:41Z

This allows developers to make a batch update to the entire control.

Resolves #38

This allows developers to make a batch update to the entire control.

connor4312 · 2017-06-08T02:08:54Z

src/state/controls/Button.ts

@@ -71,4 +71,11 @@ export class Button extends Control<IButtonData> implements IButton {
    public giveInput(input: IButtonInput): Promise<void> {
        return this.sendInput(input);
    }
+
+    public update(changedData: Partial<IButtonData>): Promise<void> {
+        if (changedData.cooldown) {


IButtonData is documented as having its cooldown be a unix timestamp, here that is broken. Prefer to keep a single unambiguous representation for all data, providing utility methods as needed for fluent shortcuts.

Also, avoid mutating function input! This is a surprising side effect.

Here im trying to understand how to indicate that for this particular method that the cooldown should be a UTC whereas setCooldown takes a duration.

Could we perhaps use an extended interface here that has duration to allow for this behavior?

Trying to make this make sense for everyone.

@connor4312

I'm not clear on the need to allow the method to take a duration at all.

Ok, so:

before: "I want to set the button's text to "connor" and the cooldown to 5 seconds"

control.setText('connor').then(() => control.setCooldown(5))

After:

control.update({ text:'connor', cooldown:<A utc what?> });

@connor4312

Connor, I want to enable batch updates as a utility method

I want to enable them easily. Control.update(..what you want to update) => Promise<void> (or <IControlData>) makes sense to me.

What doesn't is the cooldown. This is the only problem imo. As you initially were the one to update setCooldown to take the inputed duration and change it into a timestamp I would like you advisement on how to do this. Within control.update.

I do not see how creating a utility method that allows people to update multiple properties in the same service call breaks Liskov.

Within control.update.

This is the issue, this is not possible while overriding the behavior of the cooldown property. I mentioned above:

Control<T> has a method update(Partial<T>) while Button extends Control<R> while having a method update(Partial<X>)

The wikipedia page on the LSP lists:

Liskov's principle imposes some standard requirements on signatures... Contravariance of method arguments in the subtype.

Taking type X where X is not a supertype of R breaks contravariance.

Ok, so its the signature. let's change the signature: IControlUpdate, IButtonUpdate which we can document as having coolodwn in miliseconds. brb doing that.

Corrected my above message, sorry for the mistype: X must be a supertype of R, R can be a composition of X

connor4312 · 2017-06-08T02:09:08Z

src/state/controls/Control.ts

+        changedData.etag = this.etag;
+        return this.client.updateControls({
+            sceneID: this.scene.sceneID,
+            controls: [ changedData ],


connor4312 · 2017-06-08T02:10:08Z

src/state/controls/Control.spec.ts

+                controlID: buttonData.controlID,
+            },
+            buttonDiff,
+        );


Prefer the spread operator to object.assign

I prefer object.assign. Do you have a reason for its use over spreading?

It's guaranteed to be free of side effects, and is the standard we follow in all other TypeScript code/projects

Eh spread works, im just used to assign from the old days. changing sec

The way i used object.assign with the initial parameter of {} guarantees no side effects too.

ProbablePrime · 2017-06-08T03:35:07Z

@connor4312 I did a thing, with the new interface we actually prevent people shooting themselves in the foot too.

connor4312 · 2017-06-08T04:34:41Z

Still outstanding comments (in Slack DM)

SimonSchick · 2017-06-08T12:53:58Z

src/state/controls/Control.ts

@@ -104,9 +104,9 @@ export abstract class Control<T extends IControlData> extends EventEmitter imple
    /**
     * Update this control on the server.
     */
-    public update(controlUpdate: IControlUpdate): Promise<void> {
+    public update<T extends IControlUpdate>(controlUpdate: T): Promise<void> {


What is the point of this, T will always implement the interface so the statements are equivalent.

Connor says this PR breaks Liskov so im trying to fix it.

jamesbirtles · 2017-06-08T13:25:45Z

src/state/controls/Control.spec.ts

+            text: 'foobar',
+        };
+        const updatedButton = {
+            ...{


you don't need to spread an object on here, you can include the properties directly

jamesbirtles · 2017-06-08T13:29:29Z

src/state/interfaces/controls/IJoystick.ts

+export interface IJoystickUpdate extends IControlUpdate {
+    /**
+     * Updates the angle of the Joysticks direction indicator.
+     * In radians 0 - 2.


0 - 2π surely?

add control.update(), which takes a partial control

ef4f1d6

This allows developers to make a batch update to the entire control.

ProbablePrime requested review from SimonSchick, connor4312 and Mobius5150 June 8, 2017 02:05

connor4312 suggested changes Jun 8, 2017

View reviewed changes

PR Comments from Connor

22b5ec4

SimonSchick approved these changes Jun 8, 2017

View reviewed changes

ProbablePrime added 2 commits June 8, 2017 05:32

Restructure stuff

fc23d55

ss

3e56a27

This dont make sense now

4b9a32f

Ok how about generics

83e49c0

SimonSchick reviewed Jun 8, 2017

View reviewed changes

SimonSchick approved these changes Jun 8, 2017

View reviewed changes

jamesbirtles suggested changes Jun 8, 2017

View reviewed changes

Birtles fixes

20c72d7

jamesbirtles approved these changes Jun 8, 2017

View reviewed changes

connor4312 approved these changes Jun 8, 2017

View reviewed changes

ProbablePrime merged commit 510d7c2 into master Jun 8, 2017

ProbablePrime deleted the feature/batchUpdates branch June 8, 2017 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add control.update(), which takes a partial control #65

add control.update(), which takes a partial control #65

ProbablePrime commented Jun 8, 2017

connor4312 Jun 8, 2017

ProbablePrime Jun 8, 2017

ProbablePrime Jun 8, 2017

connor4312 Jun 8, 2017

ProbablePrime Jun 8, 2017 •

edited

Loading

ProbablePrime Jun 8, 2017 •

edited

Loading

connor4312 Jun 8, 2017 •

edited

Loading

ProbablePrime Jun 8, 2017

connor4312 Jun 8, 2017

ProbablePrime Jun 8, 2017

connor4312 Jun 8, 2017

connor4312 Jun 8, 2017

ProbablePrime Jun 8, 2017

connor4312 Jun 8, 2017 •

edited

Loading

ProbablePrime Jun 8, 2017

ProbablePrime Jun 8, 2017

ProbablePrime commented Jun 8, 2017

connor4312 commented Jun 8, 2017

SimonSchick Jun 8, 2017

ProbablePrime Jun 8, 2017

jamesbirtles Jun 8, 2017

ProbablePrime Jun 8, 2017

jamesbirtles Jun 8, 2017

ProbablePrime Jun 8, 2017

add control.update(), which takes a partial control #65

add control.update(), which takes a partial control #65

Conversation

ProbablePrime commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProbablePrime Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

ProbablePrime Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

connor4312 Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connor4312 Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProbablePrime commented Jun 8, 2017

connor4312 commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProbablePrime Jun 8, 2017 •

edited

Loading

ProbablePrime Jun 8, 2017 •

edited

Loading

connor4312 Jun 8, 2017 •

edited

Loading

connor4312 Jun 8, 2017 •

edited

Loading