sound: fix lint errors on conductor-migration#818
Merged
Conversation
…Error @sourceacademy/throw-runtime-error doesn't yet recognize EvaluatorParameterTypeError as assignable to RuntimeSourceError (same known gap already worked around in midi) - added the same eslint-disable + explanatory comment at each of the 6 call sites in functions.ts/index.ts. conductorToSound's invalid() helper was throwing a plain Error, not a Conductor error type at all - a real bug, not just a lint gap, since it would bypass proper student-facing error formatting. Fixed to EvaluatorRuntimeError; inlined the three call sites since the indirection through a helper function was itself what kept the linter from recognizing a type it accepts fine when thrown directly. The constructor's soundChannel wiring guard is left as a plain Error with a disable comment - a genuine internal precondition (Conductor host failed to provide the channel), never reachable from student code. Also fixed two @stylistic/member-delimiter-style warnings in recording.test.ts (semicolons vs commas in an inline type).
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
#796's Conductor migration removed AudioPlayed/SoundModuleState entirely (the old js-slang moduleContexts.sound.state pattern doesn't apply to Conductor-based modules) and reshaped Sound from a Pair to {leftWave, rightWave, duration} - several doc pages' embedded, type-checked code samples still referenced the old shapes, failing the docs build's twoslash validation: - 2-bundle/4-conventions/3-errors.md: make_sound sample used pair() from js-slang/dist/stdlib/list against the old Sound type - rewritten to return the real {leftWave, rightWave, duration} shape. - 5-advanced/context.md, 3-tabs/1-overview.md, 3-tabs/3-editing.md: all three used sound as their example bundle for the general js-slang module context / getModuleState pattern, referencing AudioPlayed/SoundModuleState which no longer exist at all. Swapped to curve, which still legitimately uses this pattern (sound moved to Conductor's own channel-based state instead) - reused curve's actual CurveModuleState/drawnCurves shape and the real Curve tab's own getModuleState usage as the reference. Verified the two rewritten make_sound/context samples against a real tsc --strict run (not just eyeballing) - both type-check clean. 4-testing/4-unit/3-mock.md and 2-bundle/1-overview/1-overview.md also reference bundle-sound but only via imports/calls that stay valid regardless of Sound's internal shape (no destructuring assuming the old pair shape) - left as-is, not build-blocking, though mock.md's AudioContext example is now semantically stale (play() no longer touches AudioContext directly) and could use a follow-up pass.
Akshay-2007-1
added a commit
that referenced
this pull request
Jul 24, 2026
…es (#819) CurveDrawn is imported into curve/types.ts from ./curves_webgl but never re-exported from it - only CurveModuleState (which uses CurveDrawn as part of its drawnCurves field type) is actually exported from @sourceacademy/bundle-curve/types. #818's context.md fix imported CurveDrawn directly, which doesn't exist at that import path (TS2459), breaking the docs build again. Fixed by deriving the element type via CurveModuleState['drawnCurves'] instead - the same pattern already used correctly in this PR's other two fixed files (3-tabs/1-overview.md, 3-tabs/3-editing.md), just missed here. Verified against a real tsc --strict run this time, not just eyeballing - all imports across all 4 previously-touched doc pages checked against their actual compiled .d.ts exports, not just what's declared in bundle source.
This file contains hidden or 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
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.
Fixes both problems blocking the
conductor-migrationdeploy after #796 merged.1. Lint errors (@martin-henz's report)
@sourceacademy/throw-runtime-errordoesn't recognizeEvaluatorParameterTypeErroras assignable toRuntimeSourceError- same known gap already worked around inmidi. 6 call sites acrossfunctions.ts/index.tshit this.Separately,
conductorToSound'sinvalid()helper was throwing a plainError, not a Conductor error type at all - a real bug, not just a lint gap, since it would bypass proper student-facing error formatting.What changed:
eslint-disable-next-line @sourceacademy/throw-runtime-error+ explanatory commentmidialready uses, at each of the 6EvaluatorParameterTypeErrorcall sites.invalid()to throwEvaluatorRuntimeErrorinstead of a plainError, and inlined the three call sites (the indirection through a helper function was itself what kept the linter from recognizing a type it accepts fine when thrown directly).soundChannelwiring guard as a plainErrorwith a disable comment - a genuine internal precondition (Conductor host failed to provide the channel), never reachable from student code.@stylistic/member-delimiter-stylewarnings inrecording.test.ts.Verified clean:
lint,tsc, all 64 tests pass.2. Docs build failure (twoslash type errors)
#796 removed
AudioPlayed/SoundModuleStateentirely (the oldjs-slangmoduleContexts.sound.statepattern doesn't apply to Conductor-based modules) and reshapedSoundfrom aPairto{leftWave, rightWave, duration}. Several doc pages' embedded, type-checked code samples still referenced the old shapes, failing the docs build's twoslash validation:2-bundle/4-conventions/3-errors.md:make_soundsample usedpair()fromjs-slang/dist/stdlib/listagainst the oldSoundtype - rewritten to return the real{leftWave, rightWave, duration}shape.5-advanced/context.md,3-tabs/1-overview.md,3-tabs/3-editing.md: all three usedsoundas their example bundle for the generaljs-slangmodule context /getModuleStatepattern, referencingAudioPlayed/SoundModuleStatewhich no longer exist at all. Swapped tocurve, which still legitimately uses this pattern (soundmoved to Conductor's own channel-based state instead) - reusedcurve's actualCurveModuleState/drawnCurvesshape and the real Curve tab's owngetModuleStateusage as the reference.Verified the two rewritten samples against a real
tsc --strictrun (not just eyeballing) - both type-check clean.4-testing/4-unit/3-mock.mdand2-bundle/1-overview/1-overview.mdalso referencebundle-soundbut only via imports/calls that stay valid regardless ofSound's internal shape (no destructuring assuming the old pair shape) - left as-is, not build-blocking, thoughmock.md'sAudioContextexample is now semantically stale (play()no longer touchesAudioContextdirectly) and could use a follow-up pass.