some smallish refactoring#4742
Conversation
|
it looks like mergeFunctionsWithConfig could be using $.extend as well |
|
LGTM. nice refactor, nice test |
f5c2e77 to
7995fa2
Compare
There was a problem hiding this comment.
This should be pretty much the same as before. I just removed a level of nesting, and then renamed some variables (I find it hard to distinguish things like blockSet vs. blockSets). Also, we previously did var block = blocks[i]; but then proceeded to use blocks[i] everywhere instead of block.
|
@davidsbailey I made some more changes to this, could you take another look? There's also a lot of text description that I'm happy to talk through in person if that would help. |
|
Actually looks like CircleCI failed on some of my new tests (I could have sworn they passed for me locally), so maybe hold off until I get those passing again. |
|
Fixed tests |
|
sorry I missed this. looking now |
There was a problem hiding this comment.
It would be helpful to add a type param for codeFunctions, and (though it may not be 100% standard) dropletConfig.blocks
There was a problem hiding this comment.
Agree. There are quite a few possible member fields I think, but probably worth while to make an attempt to at least start listing those.
There was a problem hiding this comment.
never mind the first part, I see that codeFunctions is well documented below. It might still be helpful to know what dropletConfig.blocks is
|
LGTM |
|
nice tests! |
4ea3f82 to
bfbfaeb
Compare
0578c5c Merge pull request #4742 from code-dot-org/configCleanup (Bjvanminnen) 75af0cc Merge pull request #4815 from code-dot-org/levelbuilder (Laurel) dfe9ba3 Merge pull request #4814 from code-dot-org/fix-applab-most-recent (Josh Lory) ff5f925 content changes (-laurel) (Continuous Integration) e3cd8cf When logging in, auth before `most_recent` to get the correct project (Josh Lory) 8083ad5 Merge pull request #4813 from code-dot-org/staging (Laurel) b5fa69b Merge pull request #4811 from code-dot-org/script-admin_required2 (Laurel)
Revert "Merge pull request #4742 from code-dot-org/configCleanup"
I've been spending a bunch of time dealing with droplet config recently, and find this code pretty confusing. In general, I have a couple of complaints:
(1) We seem to use this config for a bunch of different purposes (palettes, interpreter, autocompletion, etc).
(2) We pass around this config around to a bunch of different places, and it's difficult to tell where it's being mutated.
This PR does a little bit to attempt to address number 2. There are two changes in here.
The first is to have mergeCategoriesWithConfig use $.extend, which I think makes it clearer exactly what's happening (and more concise).
The second is to change generateDropletModeOptions so that instead of calling functions that mutate modeOptions, it calls functions that return it some data that it then merges into modeOptions. I also moved the logic from
setTitlesToFuncNamesForDocumentedBlocksto be inside ofgetModeOptionFunctionsFromConfig(formerlypopulateModeOptionsFromConfigBlocks).