-
Notifications
You must be signed in to change notification settings - Fork 350
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
Init perseus-score
and move answer-types
and perseus-types
#2086
Changes from 8 commits
01f3ad3
653463d
23b84d8
cdccce1
29514fc
27d6ec5
fb688d7
e7a5e47
25c4a94
42d8623
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
"@khanacademy/perseus": major | ||
"@khanacademy/perseus-score": major | ||
"@khanacademy/kmath": minor | ||
"@khanacademy/perseus-core": minor | ||
"@khanacademy/perseus-dev-ui": patch | ||
"@khanacademy/perseus-editor": patch | ||
--- | ||
|
||
Init perseus-score, move AnswerTypes from perseus to perseus-score, move perseus-types in perseus to data-schema in perseus-core |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import {number as knumber} from "@khanacademy/kmath"; | ||
import $ from "jquery"; | ||
import _ from "underscore"; | ||
|
||
import type {MathFormat} from "../perseus-types"; | ||
import {number as knumber} from "@khanacademy/kmath"; | ||
|
||
import type {MathFormat} from "@khanacademy/perseus-core"; | ||
|
||
const KhanMath = { | ||
// Simplify formulas before display | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a future PR, I think a small cleanup that'd fit these Khanmath functions into this library would be to move the functions to Probably low priority, but if you feel like it... :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added to my "post-move" cleanup ticket: https://khanacademy.atlassian.net/browse/LEMS-2776 |
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for renaming this on the move. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,3 +13,5 @@ export {libVersion} from "./version"; | |
|
||
export {Errors} from "./error/errors"; | ||
export {PerseusError} from "./error/perseus-error"; | ||
|
||
export * from "./data-schema"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea. I think it's something we could tackle as a follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added to my "post-move" cleanup ticket: https://khanacademy.atlassian.net/browse/LEMS-2776 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/** | ||
Check warning on line 1 in packages/perseus-score/.babelrc.js GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)
|
||
* HACK(somewhatabstract): Due to https://github.com/facebook/jest/issues/11741, | ||
* we need to have this file, or updating inline snapshots can fail rather | ||
* cryptically. | ||
* | ||
* We should remove this when jest is fixed. | ||
*/ | ||
module.exports = require("../../config/build/babel.config"); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* eslint-disable @typescript-eslint/no-require-imports */ | ||
/* eslint-disable import/no-commonjs */ | ||
const path = require("path"); | ||
|
||
module.exports = { | ||
rules: { | ||
"import/no-extraneous-dependencies": [ | ||
"error", | ||
{ | ||
packageDir: [__dirname, path.join(__dirname, "../../")], | ||
includeTypes: true, | ||
}, | ||
], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# @khanacademy/perseus-score | ||
|
||
Logic for scoring Perseus exercises. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
{ | ||
"name": "@khanacademy/perseus-score", | ||
"description": "Perseus score", | ||
"author": "Khan Academy", | ||
"license": "MIT", | ||
"version": "0.0.0", | ||
"publishConfig": { | ||
"access": "public" | ||
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/Khan/perseus.git", | ||
"directory": "packages/perseus-score" | ||
}, | ||
"bugs": { | ||
"url": "https://github.com/Khan/perseus/issues" | ||
}, | ||
"module": "dist/es/index.js", | ||
"main": "dist/index.js", | ||
"source": "src/index.ts", | ||
"files": [ | ||
"dist" | ||
], | ||
"scripts": {}, | ||
"dependencies": { | ||
"@khanacademy/kas": "^0.4.9", | ||
"@khanacademy/kmath": "^0.1.24", | ||
"@khanacademy/perseus-core": "3.0.5" | ||
}, | ||
"devDependencies": {}, | ||
"peerDependencies": {}, | ||
"keywords": [] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
function mark(error: string) { | ||
return "\u2603 " + error + " \u2603"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this is dumb, but I wanted to make it clear that these were not meant to be learner-facing. They're essentially enums. I thought about using numeric error codes, but I thought it would be clearer to use an enum-like system. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. One suggestion is to avoid using the snowman sigil. That has extremely special meaning in Perseus and I'd like to avoid folks seeing these values and getting confused, thinking they're widget references. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to remove the symbol altogether. It was just needless functionality and my attempt at being cute. |
||
} | ||
|
||
export const MISSING_PERCENT_ERROR = mark("MISSING_PERCENT_ERROR"); | ||
export const NEEDS_TO_BE_SIMPLIFIED_ERROR = mark( | ||
"NEEDS_TO_BE_SIMPLIFIED_ERROR", | ||
); | ||
export const APPROXIMATED_PI_ERROR = mark("APPROXIMATED_PI_ERROR"); | ||
export const EXTRA_SYMBOLS_ERROR = mark("EXTRA_SYMBOLS_ERROR"); | ||
export const WRONG_CASE_ERROR = mark("WRONG_CASE_ERROR"); | ||
export const WRONG_LETTER_ERROR = mark("WRONG_LETTER_ERROR"); | ||
export const MULTIPLICATION_SIGN_ERROR = mark("MULTIPLICATION_SIGN_ERROR"); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
export {default as KhanAnswerTypes} from "./util/answer-types"; | ||
export type {Score} from "./util/answer-types"; | ||
export { | ||
APPROXIMATED_PI_ERROR, | ||
EXTRA_SYMBOLS_ERROR, | ||
MISSING_PERCENT_ERROR, | ||
NEEDS_TO_BE_SIMPLIFIED_ERROR, | ||
WRONG_CASE_ERROR, | ||
WRONG_LETTER_ERROR, | ||
MULTIPLICATION_SIGN_ERROR, | ||
} from "./error-identifiers"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,19 @@ | ||
/* eslint-disable no-useless-escape */ | ||
import * as KAS from "@khanacademy/kas"; | ||
import {KhanMath} from "@khanacademy/kmath"; | ||
import {Errors, PerseusError} from "@khanacademy/perseus-core"; | ||
import $ from "jquery"; | ||
import _ from "underscore"; | ||
|
||
import KhanMath from "./math"; | ||
|
||
import type {PerseusStrings} from "../strings"; | ||
import { | ||
APPROXIMATED_PI_ERROR, | ||
EXTRA_SYMBOLS_ERROR, | ||
MISSING_PERCENT_ERROR, | ||
NEEDS_TO_BE_SIMPLIFIED_ERROR, | ||
WRONG_CASE_ERROR, | ||
WRONG_LETTER_ERROR, | ||
MULTIPLICATION_SIGN_ERROR, | ||
} from "../error-identifiers"; | ||
|
||
const MAXERROR_EPSILON = Math.pow(2, -42); | ||
|
||
|
@@ -100,7 +107,6 @@ const KhanAnswerTypes = { | |
createValidatorFunctional: function ( | ||
predicate: Predicate, | ||
options: any, | ||
strings: PerseusStrings, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's now using error placeholders instead of translated strings, we don't need to weave through the strings object. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! I was thinking about how this will work out on the frontend and I think your choice of string codes over numeric will be useful. If we ever have a code that's not represented on the frontend, we'll at least have a code that reveals something about what happened on the server. |
||
): (arg1: Guess) => Score { | ||
// Extract the options from the given solution object | ||
options = _.extend( | ||
|
@@ -571,13 +577,12 @@ const KhanAnswerTypes = { | |
} else if (form === "percent") { | ||
// Otherwise, an error was returned | ||
score.empty = true; | ||
score.message = strings.MISSING_PERCENT_ERROR; | ||
score.message = MISSING_PERCENT_ERROR; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So these are now the "error placeholder" vs the error message itself. There's a helper to map placeholders to translated strings. |
||
} else { | ||
if (options.simplify !== "enforced") { | ||
score.empty = true; | ||
} | ||
score.message = | ||
strings.NEEDS_TO_BE_SIMPLFIED_ERROR; | ||
score.message = NEEDS_TO_BE_SIMPLIFIED_ERROR; | ||
} | ||
// The return false below stops the looping of the | ||
// callback since predicate check succeeded. | ||
|
@@ -586,7 +591,7 @@ const KhanAnswerTypes = { | |
} | ||
if (piApprox && predicate(val, Math.abs(val * 0.001))) { | ||
score.empty = true; | ||
score.message = strings.APPROXIMATED_PI_ERROR; | ||
score.message = APPROXIMATED_PI_ERROR; | ||
} | ||
} | ||
}); | ||
|
@@ -604,7 +609,7 @@ const KhanAnswerTypes = { | |
}); | ||
if (!interpretedGuess) { | ||
score.empty = true; | ||
score.message = strings.EXTRA_SYMBOLS_ERROR; | ||
score.message = EXTRA_SYMBOLS_ERROR; | ||
return score; | ||
} | ||
} | ||
|
@@ -637,14 +642,12 @@ const KhanAnswerTypes = { | |
createValidatorFunctional: function ( | ||
correctAnswer: string, | ||
options: any, | ||
strings: PerseusStrings, | ||
): (arg1: Guess) => Score { | ||
return KhanAnswerTypes.predicate.createValidatorFunctional( | ||
...KhanAnswerTypes.number.convertToPredicate( | ||
correctAnswer, | ||
options, | ||
), | ||
strings, | ||
); | ||
}, | ||
}, | ||
|
@@ -725,7 +728,6 @@ const KhanAnswerTypes = { | |
createValidatorFunctional: function ( | ||
solution: any, | ||
options: any, | ||
strings: PerseusStrings, | ||
): (arg1: Guess) => Score { | ||
return function (guess: Guess): Score { | ||
const score = { | ||
|
@@ -787,8 +789,8 @@ const KhanAnswerTypes = { | |
score.ungraded = true; | ||
// @ts-expect-error - TS2540 - Cannot assign to 'message' because it is a read-only property. | ||
score.message = result.wrongVariableCase | ||
? strings.WRONG_CASE_ERROR | ||
: strings.WRONG_LETTER_ERROR; | ||
? WRONG_CASE_ERROR | ||
: WRONG_LETTER_ERROR; | ||
// Don't tell the use they're "almost there" in this case, that may not be true and isn't helpful. | ||
// @ts-expect-error - TS2339 - Property 'suppressAlmostThere' does not exist on type '{ readonly empty: false; readonly correct: false; readonly message: string | null | undefined; readonly guess: any; readonly ungraded: false; }'. | ||
score.suppressAlmostThere = true; | ||
|
@@ -821,7 +823,7 @@ const KhanAnswerTypes = { | |
// @ts-expect-error - TS2540 - Cannot assign to 'ungraded' because it is a read-only property. | ||
score.ungraded = true; | ||
// @ts-expect-error - TS2540 - Cannot assign to 'message' because it is a read-only property. | ||
score.message = strings.MULTIPLICATION_SIGN_ERROR; | ||
score.message = MULTIPLICATION_SIGN_ERROR; | ||
} else if (resultX.message) { | ||
// TODO(aasmund): I18nize `score.message` | ||
// @ts-expect-error - TS2540 - Cannot assign to 'message' because it is a read-only property. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"extends": "../tsconfig-shared.json", | ||
"compilerOptions": { | ||
"outDir": "./dist", | ||
"rootDir": "src", | ||
"paths": { | ||
// NOTE(kevinb): We have to repeat this here because TS doesn't do | ||
// intelligent merge of tsconfig.json files when using `extends`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just copy-pasta, I don't know what I'm doing. |
||
"@khanacademy/*": [ | ||
"../*/src" | ||
] | ||
} | ||
}, | ||
"references": [ | ||
{"path": "../kas/tsconfig-build.json"}, | ||
{"path": "../kmath/tsconfig-build.json"}, | ||
{"path": "../perseus-core/tsconfig-build.json"}, | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../../types/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exporting
sum
here introduces a duplicate function now in thatvector.ts
has anarraySum()
that is exactly the same thing.Maybe we can de-duplicate in a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to my "post-move" cleanup ticket: https://khanacademy.atlassian.net/browse/LEMS-2776