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
Remove 'edit_code' serialized property #20932
Conversation
@joshlory PTAL |
# This should just return false, but we have a few levels.js levels that use | ||
# droplet (starwars, and a few test levels). They all have level records of | ||
# type 'Blockly', so they can't override this as needed | ||
delegate :uses_droplet?, to: :game |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why delegate vs. moving the method from game.rb to here, if this is the only place it's used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence. It feels like a method that belongs in the game class since it only depends on the game type, but yeah if it's not actually used there then I suppose it make more sense to move it to blockly.rb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like reasonable cleanup. I think @cpirich's original intention was to allow any app type to work with any editor, but the reality is that we've created separate level models for Blockly and Droplet versions of a puzzle.
At some point does it make sense to rename blockly.rb
to something more general?
Yes, renaming |
# droplet (starwars, and a few test levels). They all have level records of | ||
# type 'Blockly', so they can't override this as needed | ||
def uses_droplet? | ||
%w(MazeEC ArtistEC Applab StudioEC Gamelab).include? name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be type
instead of name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah whoops, it should be game.name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name
-> game.name
fix seems like a code path we should add test coverage for.
edit_code
was a serialized property on blockly levels that (sort of) enabled droplet for that level. It didn't really work though, since some code was readingGame#uses_droplet?
instead. All the edit_code levels in scripts we actually used had a Game that used droplet, so things basically worked.I was hoping to move
uses_droplet?
entirely to the level class, but couldn't quite get rid of it thanks to the fact that all levels.js levels are of typeBlockly
, and starwars level.js levels need to use droplet (see the comment inBlockly#uses_droplet?
).At the very least, this PR ensures that
Level#uses_droplet?
will always correctly tell you if a level actually uses droplet or not, which nothing was previously able to do.