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.

Sign up for GitHub

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

SSS: Move group scoring into group scorer #1946

Merged
merged 4 commits into from
Dec 4, 2024
Merged

SSS: Move group scoring into group scorer #1946

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53d4f33
Move group scoring into group scorer
jeremywiebe Dec 4, 2024
09aa705
Changeset and cleanup
jeremywiebe Dec 4, 2024
9c6911b
Remove special case for 'group' in scorer also! :facepalm:
jeremywiebe Dec 4, 2024
581f83d
Comments
jeremywiebe Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/metal-readers-impress.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Refactor scoring for `group` widget to follow the same pattern as all other widgets
5 changes: 5 additions & 0 deletions .changeset/stupid-apricots-retire.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus-editor": patch
---

Remove debugging call in GraphSettings component
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved
1 change: 0 additions & 1 deletion packages/perseus-editor/src/components/graph-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ const GraphSettings = createReactClass({

// @ts-expect-error - TS2531 - Object is possibly 'null'. | TS2339 - Property 'value' does not exist on type 'Element | Text'.
const url = ReactDOM.findDOMNode(this.refs["bg-url"]).value; // eslint-disable-line react/no-string-refs
url; // ?
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved
if (url) {
Util.getImageSize(url, (width, height) => {
if (this._isMounted) {
Expand Down
26 changes: 6 additions & 20 deletions packages/perseus/src/renderer-util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,13 @@ export function emptyWidgetsFunctional(
return false;
}

let score: PerseusScore | null = null;
const userInput = userInputMap[id];
const scorer = getWidgetScorer(widget.type);

if (widget.type === "group") {
const scores = scoreWidgetsFunctional(
widget.options.widgets,
Object.keys(widget.options.widgets),
userInputMap[id] as UserInputMap,
strings,
locale,
);
score = Util.flattenScores(scores);
} else if (scorer) {
score = scorer(
userInput as UserInput,
widget.options,
strings,
locale,
);
}
Comment on lines -60 to -76
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: It looks like you are pulling out the special case for group from the emptyWidgetsFunctional function here, but the summary for the PR says the goal was to remove this from the scoreWidgetsFunctional one. Did you want to remove it from there as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I've fixed scoreWidgetsFunctional now too!

const score = scorer?.(
Copy link
Contributor

@Myranae Myranae Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Question/Comment: I don't remember seeing a function called like this before. So you can do a null check before calling a function by adding a question mark and a period? Very cool!

Copy link
Collaborator Author

@jeremywiebe jeremywiebe Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. It's called an "optional chaining" operator.

With it, you can safely access a property on an object, even if the object is null/undefined at runtime. The result of the call will short-circuit and be undefined at runtime. It avoids 2 extra lines of code for an if (x != null) { and the closing }.

userInputMap[id] as UserInput,
widget.options,
strings,
locale,
);

if (score) {
return Util.scoreIsEmpty(score);
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/validation.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export type PerseusExpressionRubric = {
export type PerseusExpressionUserInput = string;

export type PerseusGroupRubric = PerseusGroupWidgetOptions;
export type PerseusGroupUserInput = UserInputMap;

export type PerseusGradedGroupRubric = PerseusGradedGroupWidgetOptions;

Expand Down
5 changes: 5 additions & 0 deletions packages/perseus/src/widgets/group/group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {ApiOptions} from "../../perseus-api";
import Renderer from "../../renderer";
import {getPromptJSON as _getPromptJSON} from "../../widget-ai-utils/group/group-ai-utils";

import scoreGroup from "./score-group";

import type {PerseusGroupWidgetOptions} from "../../perseus-types";
import type {
APIOptions,
Expand Down Expand Up @@ -202,6 +204,9 @@ export default {
displayName: "Group (SAT only)",
widget: Group,
traverseChildWidgets: traverseChildWidgets,
// TODO(LEMS-2656): remove TS suppression
// @ts-expect-error: Type 'UserInput' is not assignable to type 'PerseusCSProgramUserInput'.
scorer: scoreGroup,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

hidden: true,
isLintable: true,
} satisfies WidgetExports<typeof Group>;
28 changes: 28 additions & 0 deletions packages/perseus/src/widgets/group/score-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import {scoreWidgetsFunctional} from "../../renderer-util";
import Util from "../../util";

import type {PerseusStrings} from "../../strings";
import type {PerseusScore} from "../../types";
import type {
PerseusGroupRubric,
PerseusGroupUserInput,
} from "../../validation.types";

function scoreGroup(
userInput: PerseusGroupUserInput,
options: PerseusGroupRubric,
strings: PerseusStrings,
locale: string,
): PerseusScore {
const scores = scoreWidgetsFunctional(
options.widgets,
Object.keys(options.widgets),
userInput,
strings,
locale,
);

return Util.flattenScores(scores);
}

export default scoreGroup;
Loading