feat(cli): add score playlist command#20
Conversation
Plays all examples (8-bar samples) and songs (full form) in sequence. Discovers song files from examples/ and songs/ directories automatically. Supports --examples and --songs flags to filter.
score playlist now accepts song paths as positional arguments and reads .playlist files (one path per line, # comments). No args defaults to discovering examples/ + songs/.
Replace Object.seal state hack with pure functions. Spawn node directly instead of pw-jack (platform-agnostic). Extract logEntry as a pure function. Zero let, zero mutation.
Add TSDoc block to the public playlist export per project standard. Add 7 tests covering: discovery, explicit files, .playlist files, missing file handling, 8-bar sample vs full form detection.
Add @returns, use musical voice per TSDOC_STANDARD.md.
Songs without arrangement play their pattern once — what's written is what plays. If the author wants more, they write more.
Randomize play order with a pure functional shuffle — map to random keys, sort, map back. No mutation.
Prints usage when called with --help, matching the pattern used by score new for inline help.
feat(cli): add score playlist command
Match project convention — code reads without comments. Function names and types are the documentation.
|
Because everyone likes playlists? 3 conflicts in index.ts from the new repl command. Resolution is to keep both playlist and repl imports/commands/help lines? The playlist.ts and tests have no conflicts. I'll let you merge it since I think you're busy adding more and this is all I've done so I can hear all the songs and loops. I think this follows your standards as documented and functional style (thanks for the lesson!). I added a help switch with more instructions for how a .playlist file can be used simple path/song per line. |
|
this looks good let me finish what im working on shortly and lets get it merged in and do a full review |
|
|
||
| const parseSongMeta = (filePath: string): SongMeta => { | ||
| const src = readFileSync(filePath, 'utf-8') | ||
| const bpmMatch = src.match(/bpm:\s*(\d+)/) |
There was a problem hiding this comment.
Nit: this regex matches the first bpm: occurrence in the file — including inside comments. A song with // was bpm: 120 above bpm: 140 will silently report 120. Could tighten to /^\s*bpm:\s*(\d+)/m to anchor to line-start and skip most comment cases. Not a blocker but worth hardening.
| const parseSongMeta = (filePath: string): SongMeta => { | ||
| const src = readFileSync(filePath, 'utf-8') | ||
| const bpmMatch = src.match(/bpm:\s*(\d+)/) | ||
| const keyMatch = src.match(/key:\s*'([^']+)'/) |
There was a problem hiding this comment.
This only matches single-quoted keys — key: "Amin" or key: \ will fall through to null and the key won't appear in playlist output. Could extend the pattern to accept all three quote styles: /key:\s*['"]([^'"`]+)['"`]/`
| const bpmMatch = src.match(/bpm:\s*(\d+)/) | ||
| const keyMatch = src.match(/key:\s*'([^']+)'/) | ||
|
|
||
| const sections = [...src.matchAll(/(Intro|Drop|Breakdown|Buildup|Outro)\(\s*(\d+)/g)] |
There was a problem hiding this comment.
Hardcoded to the current 5 DSL section types and scans all text including comments. Works for the songs we ship today, but won't scale if users add custom section types or if the DSL grows. A more robust approach would be to dynamically import the song file and read song.arrangement directly — the --trust flag already makes that safe in this context.
| const toEntry = (file: string): PlaylistEntry => { | ||
| const meta = parseSongMeta(file) | ||
| return { file, bars: hasArrangement(meta) ? null : 1 } | ||
| } |
There was a problem hiding this comment.
Possible bug: songs without arrangement get bars: 1, which means barDurationSec(bpm, 1) gives ~1.87s at 128 BPM. The child process is killed after ~2.87s (duration + 1) — almost certainly too short for any real song loop.
Suggest a named default constant so it is easy to tune:
const DEFAULT_LOOP_BARS = 8
return { file, bars: hasArrangement(meta) ? null : DEFAULT_LOOP_BARS }There was a problem hiding this comment.
Haha my first iteration I put that 8 bar loop in there first so I could hear everything but I didn't want to invent anything or alter anyone's arrangements so I took out the default loop for the short samples so not to be opinionated but I agree if it's just a short beat we should hear it a few times to actually hear it.
198f121
"Songs without arrangement play their pattern once — what's written
is what plays. If the author wants more, they write more. "
There was a problem hiding this comment.
Arrangements were just wired in recently, and when I built that I wasn't thinking about playlist at all. Arrangements aren't required for a song — they're optional. So bars: 1 will silently kill some songs after ~2 seconds since not all songs will have an arrangement. "What's written is what plays" is the right philosophy, but without a default loop bar count, songs without arrangement effectively play nothing. A DEFAULT_LOOP_BARS = 8 constant keeps the intent clear and gives users a usable default when no arrangement is present.
There was a problem hiding this comment.
DEFAULT_LOOP_BARS should only apply when no arrangement is present — if the song has an arrangement, it plays as written. It's purely a fallback so songs without arrangement are actually audible.
| }) | ||
| ` | ||
|
|
||
| describe('playlist', () => { |
There was a problem hiding this comment.
No test coverage for the --shuffle flag. Since shuffle uses Math.random() it's non-deterministic, but you can still verify the flag is respected — e.g. spy on Math.random, assert the queued count is the same, and confirm the shuffled suffix appears in output. Worth adding before this ships.
|
|
||
| const ordered = shuffled ? shuffle(entries) : entries | ||
|
|
||
| console.log(`Score: Playlist — ${String(ordered.length)} tracks queued${shuffled ? ' (shuffled)' : ''}`) |
There was a problem hiding this comment.
Polish / pre-publish: no pluralization logic here — 1 tracks queued is grammatically incorrect. Before publishing, worth adding a simple ternary:
`Score: Playlist — ${String(ordered.length)} ${ordered.length === 1 ? 'track' : 'tracks'} queued${shuffled ? ' (shuffled)' : ''}`Not a blocker for merge but should be fixed before a public release.
bwyard
left a comment
There was a problem hiding this comment.
Great first contribution — the command works well, the structure is clean, and the tests cover the main paths. One functional bug needs fixing before this merges; everything else I've left as inline suggestions that can follow in a separate PR.
Required fix:
The bars: 1 default for songs without arrangement (line 68) means playlist kills the child process after ~2 seconds at any BPM. A user running score playlist on their own songs would hear nothing and assume the command is broken. Please change to a named constant (e.g. DEFAULT_LOOP_BARS = 8) so it's easy to tune and the intent is clear.
Suggestions (not required for merge):
parseSongMetaregex fragilities — bpm/key/sections all have edge cases noted inline. These are fine for the songs we ship today; flagged as known limitations for when the DSL grows.--shuffletest coverage- Grammar:
1 tracks queued→ pluralization before public release
Fix the bars: 1 issue and this is good to go.
|
Cool let me play with this more when I get some time in the next couple days so I can learn from your feedback and fix what I missed. ---shuffle was kind of a last second thought and I wasn't sure how you wanted to handle switches or --help based on what I saw so far. |
|
I haven't decided how to handle switches yet, it hasn't been implemented as well as it should. We can build that into an issue. For the most part I was getting to just base CLI so I could plug into GUI. So switches and help probably should be added in. |
|
DSL update heads-up for when you return to this. The instrument API has changed since this branch was created. The old props-based style: ```js is replaced by the chain API: ```js The playlist command itself is likely fine, but any example songs or templates in this PR would need updating to the chain API style. The `@score/dsl` package exports all the chain builders (`Kick`, `Bass303`, `Pad`, etc.) — same import, new call style. This branch will also need a rebase onto `dev` before it can merge — it has diverged significantly. Happy to help rebase when you're ready to pick this back up. |
|
Thanks for this @nestorwheelock — the file-sequencer approach is clean and well-tested. This is a valid v1 of playlist. One thing worth flagging before we merge: the longer-term plan for playlist is a DSL construct rather than just a file list (tracked as t173 — unified performance model). The idea is that a live set would itself be a import { Playlist, Song } from '@score/dsl'
const kick = Kick(4).volume(0.8) // shared across songs in the set
export default Playlist(128, [
Song([kick, Bass303('A2')]),
Song([kick, Bass303('C2')]).crossfade(4),
])This doesn't block your PR — the The DSL playlist (t173) is unscoped — it needs a dedicated planning session before implementation. If you're interested in tackling that once it's scoped, that would be a great contribution. We'll note you as a potential owner when we plan it out. One blocker before merge: the branch needs a rebase onto current Te final format of playlist DSL has not been established yet. |
|
Closing this out — PR targets dev directly without a feature branch. If you want to contribute, fork the repo and open a PR from a feature branch. Happy to review. See CONTRIBUTING.md for the workflow. |
|
Keeping this open — just a note for future contributions: prefer a feature branch off dev rather than targeting dev directly. Makes it easier to review and rebase. This one's fine to land once approved. |
Summary
score playlistcommand — plays all examples and songs in sequence.playlistfiles for custom sets--shuffle/-sflag for randomized play order--help/-hfor inline usageUsage
Test plan
playlist.test.ts(40 total CLI tests pass)tsc,eslint,typecheck)const+ arrow functions, zerolet, zero classes, immutable types, no inline comments