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

Cache blockly localization options #24930

Merged
merged 4 commits into from
Sep 28, 2018
Merged

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Sep 20, 2018

First, move the localization-specific options work into a method on Blockly, a la the existing blockly_level_options method. Then, cache the results of that per-level, script, and locale.

@Hamms
Copy link
Contributor Author

Hamms commented Sep 20, 2018

@Hamms Hamms force-pushed the cache-blockly-locale-options branch from 28cd6c4 to 1318a1d Compare September 21, 2018 21:43
Changing the name with just assignment doesn't update the cache key, so
the test is still testing against the old value
@Hamms Hamms changed the title [WIP] Cache blockly localization options Cache blockly localization options Sep 27, 2018
@Hamms Hamms requested a review from joshlory September 27, 2018 18:42
Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

🎉

Is this covered by existing test coverage?

@Hamms
Copy link
Contributor Author

Hamms commented Sep 28, 2018

Hmm, good question. The fact that this change should not result in any user-visible changes is definitely covered, but we actually disable caching for unit tests so I don't think the cache hits vs misses will be tested

@Hamms Hamms merged commit f9beb69 into staging Sep 28, 2018
@Hamms Hamms deleted the cache-blockly-locale-options branch September 28, 2018 18:00
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

2 participants