Remove old old code validation, move out some logic to pxtlib/pxtblocks #9642
Conversation
…int section with multiple snippets)
| } else if (!countForBlock) { | ||
| // all instances of block are disabled | ||
| disabledBlocks.push(requiredBlockId); | ||
| } else if (countForBlock < requiredCount) { |
There was a problem hiding this comment.
Added this branch, though not yet used or thoroughly tested; to allow us to check if a block is used enough when the same block is used multiple times
| import CodeValidator = pxt.tutorial.CodeValidator; | ||
| import CodeValidatorMetadata = pxt.tutorial.CodeValidatorMetadata; | ||
| import CodeValidationResult = pxt.tutorial.CodeValidationResult; | ||
| import CodeValidationExecuteOptions = pxt.tutorial.CodeValidationExecuteOptions; | ||
|
|
||
| const defaultResult: CodeValidationResult = { | ||
| const defaultResult: () => CodeValidationResult = () => ({ |
There was a problem hiding this comment.
nit; avoiding returning the same instance, as that could be mutated accidentally / lead to issues. Also thought to just Object.freeze it but we don't use it much / that can be hard to debug afterwards as it doesn't error or anything on attempted mutations consistently (technically my understanding is that it does in strict mode, but doesn't when you're e.g. in the dev tools console which can be a nightmare)
| isValid: true, | ||
| hint: null, | ||
| }; | ||
| }); | ||
|
|
||
| export function GetValidator(metadata: CodeValidatorMetadata): CodeValidator { |
There was a problem hiding this comment.
Might still move these over and split up later on, but as mentioned in description putting that off till we have a few more use cases to see how much we need to reuse it.
| .replace( | ||
| /``` *(block|blocks)\s*\n([\s\S]*?)\n```/gim, | ||
| function (m0, m1, m2) { | ||
| hintCode.push(`{\n${m2}\n}`); |
There was a problem hiding this comment.
minor diff, we were only accounting for first snippet in a hint, probably affects no one but probably good to confirm. We might also want to include package keys in future to avoid packages changing but that's probably fine
| @@ -1190,9 +1178,13 @@ declare namespace pxt.tutorial { | |||
| execute(options: CodeValidationExecuteOptions): Promise<CodeValidationResult>; | |||
| } | |||
|
|
|||
| interface CodeValidatorBaseProperties { | |||
| enabled?: string; | |||
There was a problem hiding this comment.
Why is this a string and not a boolean? Does it have to do with getting the value from markdown?
There was a problem hiding this comment.
Yes, just matching the existing types for now; this is part of the other change I played around with but didn't commit, which would be to make it so basically all the webapp side does is link up parsing these into proper types and pass them to a validator object over in pxtlib; deferring doing that bit till we have a few more examples of validator types to work with so we can scope out what possible options / needs better and make sure it has the right api
pxtlib/tutorial.ts
Outdated
| @@ -135,7 +124,7 @@ namespace pxt.tutorial { | |||
| m2 = ""; | |||
| break; | |||
| case "tutorialValidationRules": | |||
There was a problem hiding this comment.
Or remove "tutorialValidationRules" from getMetadataRegex?
There was a problem hiding this comment.
I suppose can remove them now, this was the first file I started cleaning things up from so I hadn't made up my mind to fully clean it out yet and I guess I didn't come back to do the last few lines since they didn't really change anything
Mostly removing old, unused version of code validation, and moving some of the code validation logic to pxtblocks / pxtlib to reduce business logic in webapp layer / make it so we can add tests & expose on editor interface / etc.
I played around with a few approaches of splitting up up so the validator portion lives in pxt / only rendering related bits lived in webapp, but I'm gonna put that off till we have a few more validators / expose it elsewhere; it felt messy to pull the parts out for now, will be more clear / just as easy to separate out when we have 2 or 3 working examples.