Broadcast and list variables in Scratch Project #35

evmiguel · 2019-08-15T20:31:52Z

This PR is the partial implementation of the follow up task for accessing variables. The scope of the variables() function on ScratchProject now include broadcasts and lists, in addition to scalar variables.

Up next after this PR is querying variables based on type.

nomadtechie

@evmiguel I reviewed the code but have skipped tests in this first round. I left some comments for you and will wrap up my review when I hear back from you.

nomadtechie · 2019-08-16T14:30:56Z

src/sb3-serialize.ts

    return map(
        Object.entries(serialized),
        ([id, [name, value, isCloud]]): VariableProperties => ({
            id,
            name,
            value,
            isCloud: Boolean(isCloud), //when  isCloud is undefined
-            type: '', // this means scalar type in https://github.com/LLK/scratch-vm/blob/33ef283787d4ea9a90c3d0d069a6b97dee24f51b/src/engine/variable.js#L49
+            type: ScratchVariableTypes.SCALAR_TYPE, // this means scalar type in https://github.com/LLK/scratch-vm/blob/33ef283787d4ea9a90c3d0d069a6b97dee24f51b/src/engine/variable.js#L49


thanks for linking that scratch-vm reference - its super helpful! Alot doc tools (link jsdocs) support @link for external documentation https://jsdoc.app/tags-inline-link.html

I'm hoping the TypeScript docs support that as well..

nomadtechie · 2019-08-16T14:32:38Z

src/sb3-serialize.ts

+            name: value,
+            value,
+            type: ScratchVariableTypes.BROADCAST_MESSAGE_TYPE,
+        }),


is

name: value, value,

intentional because it looks like you have 2 of the params with the same value?

yeah - for "broadcast message" variables - name === value in the VM land

nomadtechie · 2019-08-16T14:34:38Z

src/sb-util.ts

+                },
+            );
+        }
+


I'm curious if defining setMetaSprite as an inline function inside variables is intentional? Is there any value in having it as a private function outside variables?

It is probably a copy of how I did the first pass for the others - I think this will work for now but we should come up with a better setMetaPropsWhileIterating pattern :)

gnarf · 2019-08-16T15:59:35Z

src/abstracts.ts

-    type?: string;
-    isCloud: boolean;
+    type: string;
+    isCloud?: boolean; // this only applies to scalar type varibles


I >really< prefer comments before the line they comment not at the end of it - mind doing this little nit?

There should be a lint we can turn on?

gnarf · 2019-08-16T16:05:42Z

src/sb-util.ts

+                },
+            );
+        }
+
        const stage = getSpriteMeta(this.props()).project.stage();


Logically - I think this would make more sense to me if it did all three (broadcast, list, variables) on itself first.

Rather than operating on stage for these first three, operate on this instead, then after you build taggedVariables out of those 3 iterables, you can do:

if (!this.isStage()) { taggedVariables = chain( taggedVaraibles, getSpriteMeta(this.props()).project.stage().variables().propsIterable() ); } return new VariableCollection(taggedVariables)

gnarf

1 nit - 1 more major request

gnarf · 2019-08-16T16:09:55Z

tests/data/accelerator.json

can you just reset this change (git checkout master tests/data/accelerator.json and commit it?)

gnarf · 2019-08-16T16:10:32Z

tests/sb-util.test.ts

+            expect(globalVar.global()).toBeTruthy();
+            expect(globalVar.prop('name')).toEqual('glitch_Acceleration');
+
+            // ensure that scalar vars outside of global scope are not included


separate test for this please?

gnarf · 2019-08-16T16:14:01Z

tests/sb-util.test.ts

+            expect(
+                globalListVar
+                    .prop('value')
+                    .map((v: string) => +v)


rather than +v can you just use .map(Number) ?

+v is an old trick that definitely works, but Number(v) or parseFloat(v) are much more readable / intentions are known

I generally like Number(v) cuz it also works for Boolean(v) instead of !!v and String(v) if you really want to make sure it's a string.

commit a2e0255 Merge: 63fd0cc 967b335 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:38:45 2019 -0400 Merge branch 'tmp-vars' into broadcast-list-variables commit 967b335 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:38:04 2019 -0400 merge commit 63fd0cc Merge: b09dbf4 8a50c6c Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:34:51 2019 -0400 Merge branch 'broadcast-list-variables' of github.com:bocoup/sb-util into broadcast-list-variables commit b09dbf4 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:17:03 2019 -0400 update comments commit 3fd98e0 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:16:07 2019 -0400 removing readme variable query and endline commit 8a50c6c Merge: c960d46 0686f35 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:31:42 2019 -0400 Merge branch 'broadcast-list-variables' of github.com:bocoup/sb-util into broadcast-list-variables commit d0a4e7c Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Sun Aug 18 17:51:51 2019 -0400 flippinf global logic on variables and lists commit c960d46 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:17:03 2019 -0400 update comments commit fc571ad Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:16:07 2019 -0400 removing readme variable query and endline commit 0686f35 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:17:03 2019 -0400 update comments commit f1c0f78 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:16:07 2019 -0400 removing readme variable query and endline commit 820d6b1 Merge: b09c120 9a48bed Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Mon Aug 19 11:15:07 2019 -0400 Merge branch 'broadcast-list-variables' of github.com:bocoup/sb-util into broadcast-list-variables commit b09c120 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Sun Aug 18 17:51:51 2019 -0400 flippinf global logic on variables and lists commit 07686e9 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:30:16 2019 -0400 set meta sprite in separate function commit a4ef817 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:24:35 2019 -0400 list variable in local and global scope accessing commit b3ff635 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 15:46:00 2019 -0400 breaking out variable tests commit 9dde03e Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 14:26:43 2019 -0400 adding support for broadcast variables commit e44ad7c Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:45:57 2019 -0400 break out tests commit 74628c6 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:38:23 2019 -0400 nit comment commit aff33d5 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:36:34 2019 -0400 broadcast vars break out commit 8e182bd Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:31:37 2019 -0400 breaking out list vars into its own method commit 3087648 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:30:16 2019 -0400 set meta sprite in separate function commit bc6cd57 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:24:35 2019 -0400 list variable in local and global scope accessing commit 9da2eee Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 15:46:00 2019 -0400 breaking out variable tests commit 44fec5d Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 14:26:43 2019 -0400 adding support for broadcast variables commit 9c3ff2a Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Wed Aug 14 16:18:05 2019 -0400 updating docblock to include broadcasts and lists commit 9a48bed Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Sun Aug 18 17:51:51 2019 -0400 flippinf global logic on variables and lists commit 8126e59 Merge: 9e7a162 d5786ba Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Sun Aug 18 17:38:19 2019 -0400 Merge branch 'broadcast-list-variables' of github.com:bocoup/sb-util into broadcast-list-variables commit 9e7a162 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:45:57 2019 -0400 break out tests commit f380ae6 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:38:23 2019 -0400 nit comment commit e1da0de Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:36:34 2019 -0400 broadcast vars break out commit 1c78b05 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:31:37 2019 -0400 breaking out list vars into its own method commit 3ee02e9 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:30:16 2019 -0400 set meta sprite in separate function commit 62fc4c4 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:24:35 2019 -0400 list variable in local and global scope accessing commit a2469c8 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 15:46:00 2019 -0400 breaking out variable tests commit 7c7957d Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 14:26:43 2019 -0400 adding support for broadcast variables commit 95aa3ae Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Wed Aug 14 16:18:05 2019 -0400 updating docblock to include broadcasts and lists commit d5786ba Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:45:57 2019 -0400 break out tests commit a6f0bf9 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:38:23 2019 -0400 nit comment commit d680d01 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:36:34 2019 -0400 broadcast vars break out commit d82d04b Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Fri Aug 16 16:31:37 2019 -0400 breaking out list vars into its own method commit 258064e Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:30:16 2019 -0400 set meta sprite in separate function commit ff44d8e Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 16:24:35 2019 -0400 list variable in local and global scope accessing commit 2fc720c Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 15:46:00 2019 -0400 breaking out variable tests commit 9786fba Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Thu Aug 15 14:26:43 2019 -0400 adding support for broadcast variables commit c025ad1 Author: Erika Miguel-Yeo <erika@bocoup.com> Date: Wed Aug 14 16:18:05 2019 -0400 updating docblock to include broadcasts and lists

evmiguel requested review from gnarf, mzgoddard, nomadtechie and ktbee August 15, 2019 20:31

evmiguel self-assigned this Aug 15, 2019

nomadtechie reviewed Aug 16, 2019

View reviewed changes

gnarf reviewed Aug 16, 2019

View reviewed changes

gnarf suggested changes Aug 16, 2019

View reviewed changes

gnarf reviewed Aug 16, 2019

View reviewed changes

evmiguel force-pushed the broadcast-list-variables branch from a2e0255 to 11e7bfa Compare August 19, 2019 15:57

evmiguel merged commit 7b93764 into master Aug 19, 2019

evmiguel deleted the broadcast-list-variables branch August 19, 2019 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broadcast and list variables in Scratch Project #35

Broadcast and list variables in Scratch Project #35

evmiguel commented Aug 15, 2019 •

edited

Loading

nomadtechie left a comment

nomadtechie Aug 16, 2019 •

edited

Loading

nomadtechie Aug 16, 2019

gnarf Aug 16, 2019

nomadtechie Aug 16, 2019

gnarf Aug 16, 2019

gnarf Aug 16, 2019

mzgoddard Aug 16, 2019

gnarf Aug 16, 2019 •

edited

Loading

gnarf left a comment

gnarf Aug 16, 2019

gnarf Aug 16, 2019

gnarf Aug 16, 2019

Broadcast and list variables in Scratch Project #35

Broadcast and list variables in Scratch Project #35

Conversation

evmiguel commented Aug 15, 2019 • edited Loading

nomadtechie left a comment

Choose a reason for hiding this comment

nomadtechie Aug 16, 2019 • 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

gnarf Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

gnarf left a 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

evmiguel commented Aug 15, 2019 •

edited

Loading

nomadtechie Aug 16, 2019 •

edited

Loading

gnarf Aug 16, 2019 •

edited

Loading