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

Seed and serialize spritelab programming expressions #44071

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

bethanyaconnor
Copy link
Contributor

@bethanyaconnor bethanyaconnor commented Dec 15, 2021

Seed and serialize spritelab!

My proposal, which is implemented in this PR, is to go with the same seed/serialize logic as the rest of the labs, for a few reasons:

  • There are spritelab blocks that are not represented in the blocks table, which need a config in config/programming_expressions/spritelab. For me, this is the most compelling reason
  • Consistency across all programming environments makes this feature simpler

The biggest downside is that this information already lives in a bunch of different places (in the blocks table, in apps/, in the blockly repo) and we're adding another place. For me, consistency and simplicity of this feature outweighed the desire to avoid duplication of this information, but I've open to debate here.

I considered writing this data to the files in config/blocks/GamelabJr but didn't because:

  • Like I said above, this doesn't work for all spritelab blocks
  • It like it would muddle the two models too much

That said, I am planning on adding a field block_name to ProgrammingExpression that would at least create a pointer between the two models (not as an association because, again, not all blocks have an entry in blocks).

Thoughts? I can write this up as a doc if folks would prefer that, but wasn't sure it quite warranted one.

For review, I would recommend viewing the commit with code changes with whitespace hidden then skim the commit that serializes the programming expressions.

Testing story

serialized all programming expressions, committed those files, deleted all programming expressions, reseeded, reserialized, and ensured no diffs. #44019 added a test for this logic

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@bethanyaconnor bethanyaconnor marked this pull request as ready for review December 16, 2021 00:49
@bethanyaconnor bethanyaconnor changed the title Seed serialize spritelab Seed and serialize spritelab programming expressions Dec 16, 2021
@bethanyaconnor bethanyaconnor requested review from dmcavoy and a team December 16, 2021 00:49
@davidsbailey
Copy link
Member

Hi Bethany, I have some initial questions about the overall approach. it sounds like a good choice, but I am trying to see if I understand what this means for adding more blocks in the future. are these points correct?

  1. if I add a spritelab block in the blocks table, I have to remember to also add an entry in config/programming_expressions/spritelab. if I don't, then this block will still work, but won't have a documentation page.
  2. if I add a spritelab block that is not in the blocks table by creating an entry in config/programming_expressions/spritelab, then there is no new way to get into a bad state, because creating that entry will create both the block and the documentation page.

for (1), a follow-up question is how bad will that situation be -- are there things that we should make sure don't break in this scenario? applab toolbox blocks have links to their docs, but I don't see any such links on https://studio.code.org/projects/spritelab , so I'm not sure that concern applies here.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed write-up, Bethany! This approach and the actual code change both look good to me.

@@ -10,5 +10,6 @@
"name": "text"
}
],
"syntax": "button(id, text)"
"syntax": "button(id, text)",
"video_key": "alg_6_variables"
Copy link
Member

Choose a reason for hiding this comment

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

is this change to an applab block expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh oops 😅

@bethanyaconnor
Copy link
Contributor Author

if I add a spritelab block in the blocks table, I have to remember to also add an entry in config/programming_expressions/spritelab. if I don't, then this block will still work, but won't have a documentation page.

Yep, that's correct.

if I add a spritelab block that is not in the blocks table by creating an entry in config/programming_expressions/spritelab, then there is no new way to get into a bad state, because creating that entry will create both the block and the documentation page.

No, the way this code works now is that the tables are completely separate. There are spritelab blocks not in the blocks table already, so there is not a new way to get into a bad state, but adding a programming expression does not create a new block.

for (1), a follow-up question is how bad will that situation be -- are there things that we should make sure don't break in this scenario? applab toolbox blocks have links to their docs, but I don't see any such links on https://studio.code.org/projects/spritelab , so I'm not sure that concern applies here.

The problem will all of this is that there is no one source of truth for blocks. In applab, blocks are defined in https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/applab/dropletConfig.js and I guess people have been remembering to add corresponding documentation into curriculumbuilder when blocks are added? I'm not sure if there are blocks without corresponding documentation

@bethanyaconnor bethanyaconnor requested a review from a team December 16, 2021 18:27
@davidsbailey
Copy link
Member

if I add a spritelab block that is not in the blocks table by creating an entry in config/programming_expressions/spritelab, then there is no new way to get into a bad state, because creating that entry will create both the block and the documentation page.

No, the way this code works now is that the tables are completely separate. There are spritelab blocks not in the blocks table already, so there is not a new way to get into a bad state, but adding a programming expression does not create a new block.

how are new blocks added that are not in the blocks table? apologies, I am forgetting some of the background here.

The problem will all of this is that there is no one source of truth for blocks. In applab, blocks are defined in https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/applab/dropletConfig.js and I guess people have been remembering to add corresponding documentation into curriculumbuilder when blocks are added? I'm not sure if there are blocks without corresponding documentation

Yes, agreed, the system is not set up in a way that makes it easy to have a single source of truth for these. I think it is reasonable for us to not tackle that now, though. If our goal is parity with Curriculum Builder, then you've taken away one place each block needs to be added and replaced it with another place, so I think we are good in that sense.

@dmcavoy
Copy link
Contributor

dmcavoy commented Dec 16, 2021

I think this makes sense. We have already ended up with different places that define the blocks and document the blocks for the other labs like applab. It seems like at least the fact that spritelab will be consistent with the other areas is a win. I still wish we had a way to better validate and check that blocks added in one place get added in the other as well but with spritelab since there is no connection from the programming environment to the documentation on the blocks is probably not as big of a deal anyway

@bethanyaconnor
Copy link
Contributor Author

how are new blocks added that are not in the blocks table? apologies, I am forgetting some of the background here.

I had to look this up last week so definitely no apologies needed. The short answer is that most of them are defined in the code-dot-org/blockly repo.

The long answer with probably too much detail:
I ran

ProgrammingEnvironment.find_by_name('spritelab').programming_expressions.
  reject {|exp| Block.find_by_name(exp.key) || Block.find_by_name("gamelab_" + exp.key) }.
  map {|exp| [exp.key, exp.syntax, exp.category] }.
  sort_by {|a| a[2] }

on the rails console to get the spritelab expressions not represented in the blocks table (some have the gamelab_ prefix, some don't).

It gave me (programming expression key, function name, category):

[["codestudio_definingFunction", "procedures_defnoreturn", "Functions"], ["codestudio_callingFunction", "procedures_callnoreturn", "Functions"], ["codestudio_andOrOperator", "logic_operation", "Logic"], ["codestudio_ifStatement", "controls_if_if", "Logic"], ["codestudio_trueFalse", "logic_boolean", "Logic"], ["codestudio_comparisonOperator", "logic_compare", "Logic"], ["codestudio_repeat", "controls_repeat", "Loops"], ["codestudio_for", "controls_for", "Loops"], ["codestudio_arithmeticOperator", "math_arithmetic", "Math"], ["codestudio_number", "math_number", "Math"], ["codestudio_randomInt", "math_random_int", "Math"], ["bounce-off", "bounceOff", "Sprites"], ["codestudio_spriteName", "sprite_variables_get", "Sprites"], ["codestudio_join", "text_join", "Text"], ["codestudio_string", "text", "Text"], ["codestudio_variableName", "variables_get", "Variables"], ["codestudio_changeVariable", "math_change", "Variables"], ["codestudio_setVariable", "variables_set", "Variables"]]

Of these, most were defined in the files in https://github.com/code-dot-org/blockly/tree/main/blocks. codestudio_spriteName is special and defined in spritelab/blocks.js. bounce-off is actually in blocks but as gamelab_bounceOff, so my naive search didn't catch it.

@davidsbailey
Copy link
Member

Thank you Bethany, great explanation!

@bethanyaconnor bethanyaconnor merged commit 9eb539a into staging Jan 4, 2022
@bethanyaconnor bethanyaconnor deleted the seed-serialize-spritelab branch January 4, 2022 18:55
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.

None yet

4 participants