Rename widget validators to use scoring phrasing #1848
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (76e6120) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1848If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1848 |
|
Size Change: +21 B (0%) Total Size: 866 kB
ℹ️ View Unchanged
|
Myranae
commented
Nov 13, 2024
| export function csProgramValidator( | ||
| state: PerseusCSProgramUserInput, | ||
| ): PerseusScore { | ||
| export function scoreCSProgram(state: PerseusCSProgramUserInput): PerseusScore { |
There was a problem hiding this comment.
Looks like this scoring file doesn't do the export default at the bottom. Should I update it to export like the others?
There was a problem hiding this comment.
Yes, please. We mix between export default and exporting named functions in Perseus, but we should at least be consistent across the scoring function/files.
There was a problem hiding this comment.
Jeremy and I disagree; I think export function is better and he advocates for having it at the end. I think it's best to be consistent more than anything.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
jeremywiebe
approved these changes
Nov 13, 2024
.eslintrc.js
Outdated
There was a problem hiding this comment.
Thanks for fixing the linting also!
| export function csProgramValidator( | ||
| state: PerseusCSProgramUserInput, | ||
| ): PerseusScore { | ||
| export function scoreCSProgram(state: PerseusCSProgramUserInput): PerseusScore { |
There was a problem hiding this comment.
Yes, please. We mix between export default and exporting named functions in Perseus, but we should at least be consistent across the scoring function/files.
| @@ -28,13 +28,12 @@ import type { | |||
| * - For each answer: | |||
| * ~ Try to validate the user's input against the answer. The answer is | |||
| * expected to parse. | |||
| * ~ If the user's input validates (the validator judges it "correct"), we've | |||
| * matched and can stop considering answers. | |||
There was a problem hiding this comment.
Did you delete this line by accident?
There was a problem hiding this comment.
Yep, I accidentally modified this comment before realizing it was related to khanAnswerType instead of the SSS validators. Returning it now! Thanks for pointing it out!!
handeyeco
approved these changes
Nov 13, 2024
| it("returns a pointless object", () => { | ||
| const result = noopValidator(); | ||
| const result = scoreNoop(); |
There was a problem hiding this comment.
nit: score[Widget] makes sense, but I don't think scoreNoop really makes sense. I think noopScorer is better?
There was a problem hiding this comment.
I think I was going for consistency with the naming of the scoring related functions, but that seems fine with me. Might be a good idea to keep the file name the way it is to conform to the eslint rules though. What do you think?
There was a problem hiding this comment.
We talked in messages and decided to leave it as is so it is consistent and conforms to the eslint rules.
| export function csProgramValidator( | ||
| state: PerseusCSProgramUserInput, | ||
| ): PerseusScore { | ||
| export function scoreCSProgram(state: PerseusCSProgramUserInput): PerseusScore { |
There was a problem hiding this comment.
Jeremy and I disagree; I think export function is better and he advocates for having it at the end. I think it's best to be consistent more than anything.
| @@ -28,13 +28,12 @@ import type { | |||
| * - For each answer: | |||
| * ~ Try to validate the user's input against the answer. The answer is | |||
| * expected to parse. | |||
| * ~ If the user's input validates (the validator judges it "correct"), we've | |||
| * matched and can stop considering answers. | |||
There was a problem hiding this comment.
Did you mean to delete this line?
There was a problem hiding this comment.
Nope, definitely was unintended. I accidentally modified this comment before realizing it was related to khanAnswerType instead of the SSS validators. Returning it now! Thanks for pointing it out!!
Merged
Myranae
pushed a commit
that referenced
this pull request
Nov 14, 2024
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus@41.3.0 ### Minor Changes - [#1848](#1848) [`55371ded7`](55371de) Thanks [@Myranae](https://github.com/Myranae)! - Rename all validator functions and related code to reference scoring instead - [#1754](#1754) [`77f1bf98f`](77f1bf9) Thanks [@MikeKlemarewski](https://github.com/MikeKlemarewski)! - Adds the getPromptJSON utility method to the renderer and every widget. It can be used to get a JSON representation of the exercise to pass to an LLM. ### Patch Changes - [#1844](#1844) [`092c81f6c`](092c81f) Thanks [@Myranae](https://github.com/Myranae)! - Update Radio's userInput and rubric objects and types - [#1837](#1837) [`80f0480e6`](80f0480) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Type widget exports using 'satisfies' keyword instead of 'as' - [#1785](#1785) [`32b27322c`](32b2732) Thanks [@benchristel](https://github.com/benchristel)! - Internal: add a parsing layer to typecheck PerseusItem data at runtime (ADR #773). The parsing code is not used yet. - [#1838](#1838) [`08fa66103`](08fa661) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Introduce a PerseusArticle type - [#1825](#1825) [`3dd1fa5ce`](3dd1fa5) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Adding stronger typing to expression editor issues. - [#1847](#1847) [`dcb4b27b3`](dcb4b27) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Labels] Allow the usage of `{$}` to denote actual dollar signs within locked label TeX - [#1850](#1850) [`61dbd4a2c`](61dbd4a) Thanks [@MikeKlemarewski](https://github.com/MikeKlemarewski)! - Export WidgetPromptJSON type so it is accessible - [#1842](#1842) [`cf2ea471e`](cf2ea47) Thanks [@MikeKlemarewski](https://github.com/MikeKlemarewski)! - Fix function signature of getUserInputFromProps for radio widget ## @khanacademy/perseus-editor@14.11.1 ### Patch Changes - [#1825](#1825) [`3dd1fa5ce`](3dd1fa5) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Adding stronger typing to expression editor issues. - [#1847](#1847) [`dcb4b27b3`](dcb4b27) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Labels] Allow the usage of `{$}` to denote actual dollar signs within locked label TeX - Updated dependencies \[[`092c81f6c`](092c81f), [`80f0480e6`](80f0480), [`32b27322c`](32b2732), [`08fa66103`](08fa661), [`55371ded7`](55371de), [`3dd1fa5ce`](3dd1fa5), [`dcb4b27b3`](dcb4b27), [`61dbd4a2c`](61dbd4a), [`77f1bf98f`](77f1bf9), [`cf2ea471e`](cf2ea47)]: - @khanacademy/perseus@41.3.0 Author: khan-actions-bot Reviewers: Myranae Required Reviewers: Approved By: Myranae Checks: ⏭️ Publish npm snapshot, ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x) Pull Request URL: #1840
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
In preparation of splitting out the valid/invalid logic from the scoring logic, this renames the widget validator functions and related code to use phrasing related to scoring instead of validation. The goal is to prevent confusion when we add actual validation logic to validate a user's input before sending to the server and right before scoring on the server.
Issue: LEMS-2593
Test plan: