Skip to content

Conversation

@kchadha
Copy link
Contributor

@kchadha kchadha commented Dec 18, 2018

Resolves #49 which was causing save/load failures for 2.0 and 3.0 projects.

@kchadha
Copy link
Contributor Author

kchadha commented Dec 18, 2018

@picklesrus and I discovered an issue with saving/loading a project from 2.0 where a sprite was named 'stage'. Turns out that we can reproduce this error (completely separate from 2.0) where naming a sprite 'stage' or 'Stage' in the 3.0 editor does not allow save/load because the sb3 fails validation. This fix will need to be deployed to scratch-projects and scratch-analysis (in addition to being updated in scratch-vm).

@kchadha
Copy link
Contributor Author

kchadha commented Dec 18, 2018

Hmmm for some reason commit lint is failing on travis (but running npm run commitmsg locally is not failing for me.

It looks like this is a problem with the node version. Switching to node 11 locally and running npm run commitmsg gives me the same error I see in the travis build...

Copy link

@joker314 joker314 left a comment

Choose a reason for hiding this comment

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

Question about _stage_

"name": {
"type": "string",
"not": {"enum": ["Stage", "stage"]}
"type": "string"

Choose a reason for hiding this comment

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

Should validation not fail if you try to name a sprite _stage_, or is that handled elsewhere?

Suggested change
"type": "string"
"type": "string"
"not": {"enum": ["_stage_"]}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joker314, good catch! It looks like both the 2.0 and 3.0 editors prevent renaming a sprite to _stage_ so that should be included in the validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching that @joker314.

add test fixture and unit test for this change
Copy link
Contributor

@thisandagain thisandagain left a comment

Choose a reason for hiding this comment

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

This looks good to me. It would be nice to test the error condition with a test fixture that has a sprite called _stage_, but other than that I think this is good to go.

"name": {
"type": "string",
"not": {"enum": ["Stage", "stage"]}
"type": "string"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching that @joker314.

@kchadha
Copy link
Contributor Author

kchadha commented Dec 19, 2018

@thisandagain, added the fix that @joker314 suggested and added a test fixture in the invalid sub directory for it.

I also updated the commitlint package (verifying locally that it works with both node 8 and 11), so now the build is not failing on travis anymore.

@thisandagain thisandagain assigned kchadha and unassigned thisandagain Dec 19, 2018
@kchadha kchadha merged commit 1857921 into scratchfoundation:master Dec 19, 2018
@kchadha
Copy link
Contributor Author

kchadha commented Dec 19, 2018

🎉 This PR is included in version 4.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants