-
Notifications
You must be signed in to change notification settings - Fork 481
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
Script dsl lesson rename #34921
Script dsl lesson rename #34921
Conversation
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 awesome! The curriculum team will definitely appreciate a heads up on this.
one minor request is to remove "WIP" from the PR description.
you asked about deployment issues. I think it's generally ok to be a bit more lax about deployment issues which only affect curriculum writers, since they generally have to close out of levelbuilder during the deploy and reload after, so there's less risk of server + client code getting out of sync. plus, giving them a warning that this is happening will help them know that they might need to reload.
@@ -227,7 +227,7 @@ def populate_cache_and_disconnect_db | |||
scripts, _ = Script.setup([script_file_all_properties]) | |||
script = scripts.first | |||
|
|||
assert_equal 21, script.properties.keys.length | |||
assert_equal 20, script.properties.keys.length |
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 changing?
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.
Yea, I removed hideable_stages
from the script_dsl parse output in this change also, forgot to call that out.
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.
No worries - Was just scanning for anything that jumped out. All looks good then!
The net result should be that the script DSL uses "lesson" instead of "stage.
The commits are hopefully separated logically so you can review each part of the change in isolation.
I don't think there should be any deployment issues but it'd be great to have your take on that too.
Testing story
Reviewer Checklist: