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

Cast sprite name to string before getSpriteTargetByName #2038

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

ktbee
Copy link
Contributor

@ktbee ktbee commented Mar 6, 2019

Resolves

#1743

Proposed Changes

For blocks that use getSpriteTargetByName, cast the sprite name to a string before passing to getSpriteTargetByName. These are the blocks that are fixed by this change:

  • Go to
  • Glide to
  • Point towards
  • Create clone of
  • Distance to
  • X position of
  • Touching sprite

Reason for Changes

Currently getSpriteTargetByName doesn't cast sprite names to a string, so any sprites with a number for a name might not be found if a number is the type of the value passed to getSpriteTargetByName. As discussed in #1743, it would generally be better to have this string casting done in one place instead of having the same logic repeated in multiple blocks. I am doing this non-DRY solution for now however because @mzgoddard has plans as part of his performance optimizations to make the string casting happen elsewhere in the VM in one place. That work is being tracked in #2013

Test Coverage

I added sprite-number-name.sb2 as one of the tests run by execute.js.

Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

Change requested in the code comments.

@ktbee ktbee force-pushed the cast-sprite-names-to-string branch from f6297bc to bdc050a Compare March 7, 2019 20:55
@ktbee ktbee requested a review from kchadha March 7, 2019 20:57
@ktbee ktbee force-pushed the cast-sprite-names-to-string branch from bdc050a to c37745e Compare March 7, 2019 21:09
@ktbee ktbee removed their assignment Mar 7, 2019
Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@kchadha kchadha assigned ktbee and unassigned kchadha Mar 8, 2019
@ktbee ktbee merged commit 19a4329 into scratchfoundation:develop Mar 8, 2019
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.

2 participants