Skip to content

Include New York Times Midi#319

Open
zachbogart wants to merge 2 commits intothisisparker:mainfrom
zachbogart:zb-20260303-nytMidi
Open

Include New York Times Midi#319
zachbogart wants to merge 2 commits intothisisparker:mainfrom
zachbogart:zb-20260303-nytMidi

Conversation

@zachbogart
Copy link
Copy Markdown

@clodpated
Copy link
Copy Markdown

✅ What's good

  • Follows established patterns perfectly — the class is nearly identical in structure to NewYorkTimesMiniDownloader, which is exactly right. Same inheritance, same inherit_settings="nyt", same override pattern for url_from_date, matches_url, and find_latest.
  • API endpoints are real and working — both the oracle (/v2/oracle/midi.json) and puzzle data (/v6/puzzle/midi/{date}.json) return valid data with the same JSON schema as daily/mini.
  • All three access modes work — I tested --latest, --date, and direct URL parsing. All produce valid .puz files.
  • Auth inheritance works — correctly reuses the parent NYT-S token via inherit_settings="nyt".
  • No parse_xword override needed — the Midi data format is identical to daily, so the parent parser handles it fine (tested with a 9×11, 38-clue puzzle).

⚠️ One concern: the nytd command keyword
nytd could easily be misread as "NYT Daily" rather than "NYT Midi." The existing convention is:

nyt = daily
nytm = mini
nytv = variety
Since nytm is taken, nytd is a compromise — but it's confusing. Something like nytmid or midi would be clearer

Also for future PRs the PR description here was insufficient and required me to do a lot of inferring, when contributing to public repos a complete PR body would be appreciated.

Suggesting approval once nytd is changed to nytmid.

@zachbogart
Copy link
Copy Markdown
Author

I can understand the confusion in keyword choice. nytd was chosen because it follows a similar convention in the rest of the project to use keywords with common prefixes and minimal letters. To this point:

  • nytmid would work, but it is much longer than other keywords.
  • midi on its own does not specify the origin

I went with nytd because it seems to follow the other keyword progressions, but it is true that it could be confusing when paired with the other options. I went with "nyt" as a prefix, then since "m" was used, went with "d" for the "d" in "midi".

It's a challenge to balance. There could be a future discussion into more descriptive keywords across the tool, but I think nytd works well.

@clodpated
Copy link
Copy Markdown

That's fair it's a hard thing to truncate well. Recommend approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants