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.

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

Extend check/transform to support nearly all types of [[Link]] shortcuts in Commentary #597

Open
wants to merge 2 commits into
base: preview
Choose a base branch
from

Conversation

recordcrash
Copy link
Contributor

@recordcrash recordcrash commented Jan 27, 2025

Closes #579 and probably another one I can't find.

Before this PR, [[TrackName]] had hardcoded special handling for Tracks across the codebase. In Commentary bodies, you could use [[Sburban Jungle]] and have it resolve to [[track:sburban-jungle]], but you couldn't simply use [[Michael Guy Bowman]] or a group/album name.

This adds support to [[Syntax]] to groups, albums and artists in Commentary, though extending it to other fields like Lyrics if we need it should be easy.

image
image

Absolutely do not merge this before thorough review, for reasons.

For one, the priority currently goes Track > Artist > Album > Group (we then check for Artist Alias, erroring if there's a match), based on an estimated order of number of appearances. When developing, I originally crashed against "Moonsweater", which matched Whimsy's "moonsweater" alias, which luckily this order fixes. Since all the current [[References]] are to tracks, I think this change is fully backwards compatible and we don't need to touch hsmusic-data at all. I haven't seen anything wrong while going through random Commentaries, but we should keep checking.

Also, currently we have this order defined in two places, checks.js and transformContent.js, which is Bad. I couldn't find a good place to define a constant or even a variable that will eventually make it to both places. If it exists, feel free to tell me where and I'll fix that. Alternatively maybe we can dynamically fetch all the names of Things that implement getMatchableNames, but if there was an easy way to do that I couldn't find it.

Finally, while all checks pass, it's completely possible I have broken something (especially in error handling, which I still barely understand) and made them more permissive, so this should be checked too, though I haven't seen anything wrong, and I did some manual tests.

Good luck reviewing this!

@recordcrash recordcrash force-pushed the makin/universal-find-in-commentary branch from 93c39de to b65249c Compare January 27, 2025 16:41
Copy link
Member

@towerofnix towerofnix left a comment

Choose a reason for hiding this comment

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

Couple of style nits for bindFindThing! We haven't closely reviewed the rest yet (or tested that everything works lol)

Comment on lines 156 to 157
for (const boundFindFunc of keylessReplacerOrder.map(key => boundFind[key])) {
const thing = boundFindFunc(unknownRef, {mode: 'quiet'});
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to iterate directly over keylessReplacerOrder up earlier...

for (const tryKey of keylessReplacerOrder) {
const attempt = find[tryKey](replacerValue, {mode: 'quiet'});

...can you do the same here, too?

Suggested change
for (const boundFindFunc of keylessReplacerOrder.map(key => boundFind[key])) {
const thing = boundFindFunc(unknownRef, {mode: 'quiet'});
for (const key of keylessReplacerOrder) {
const thing = boundFind[key](unknownRef, {mode: 'quiet'});

// TODO: match and error on other "alternate names" like the ones for tracks and albums
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Just some code style things for this function: Could you try not to needlessly cut out blank lines between C blocks? Here's how I'd format what you have atm, dropping the // todo at the end:

function bindFindThing(boundFind) {
  return unknownRef => {
    // Find findable Things in the order defined in keylessReplacerOrder
    for (const boundFindFunc of keylessReplacerOrder.map(key => boundFind[key])) {
      const thing = boundFindFunc(unknownRef, {mode: 'quiet'});
      if (thing) {
        return boundFindFunc(unknownRef);
      }
    }

    // If we still didn't match anything, check if an artist alias was misused
    const artistAlias = boundFind.artistAlias(unknownRef, {mode: 'quiet'});
    if (artistAlias) {
      // No need to check if the original exists here. Aliases are automatically
      // created from a field on the original, so the original certainly exists.
      const original = artistAlias.aliasedArtist;
      throw new Error(`Reference ${colors.red(unknownRef)} is to an alias, should be ${colors.green(original.name)}`);
    }

    throw new Error(`Reference ${colors.red(unknownRef)} didn't match anything`);
  }
}

Comment on lines 167 to 168
const original = artistAlias.aliasedArtist;
throw new Error(`Reference ${colors.red(unknownRef)} is to an alias, should be ${colors.green(original.name)}`);
Copy link
Member

Choose a reason for hiding this comment

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

We don't really like making single-use variables that are literally just equivalent to a plain property access. Like, maybe "original" is more meaningful to you here, but everywhere else it's called <Artist>.aliasedArtist, so we prefer to use the latter syntax for consistency's sake.

We'd also split this string interpolation into a couple lines for legibility, like so:

Suggested change
const original = artistAlias.aliasedArtist;
throw new Error(`Reference ${colors.red(unknownRef)} is to an alias, should be ${colors.green(original.name)}`);
throw new Error(
`Reference ${colors.red(unknownRef)} is to an alias, ` +
`should be ${colors.green(artistAlias.aliasedArtist.name)}`);

@@ -150,6 +150,28 @@ function bindFindArtistOrAlias(boundFind) {
};
}

function bindFindThing(boundFind) {
return unknownRef => {
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to veto if you disagree, but we'd just call this unknownRef parameter "ref"! It's only "unknown" until you start constraining information about it by performing, well, the body of this function. What remains consistent throughout the function is just that it is a reference (and is the same reference).

We don't really use parameter names as critical, complete descriptions of what goes into a function, so much as just a description of what sort of thing to put there, though, so like this is probably a POV difference

The rationale for us is that the name of the function (or, your association of meaning w/ that name) is sufficient to qualify what goes into it; parameter names just explain to you the order they belong in. "bindFindThing returns a function which accepts ref, and yeah that checks out, bindFindThing returns a bound find.thing, all find.* functions operate on a reference, gotcha."

@recordcrash recordcrash force-pushed the makin/universal-find-in-commentary branch from b65249c to 956684b Compare January 28, 2025 14:39
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.

Add 'Always Reference By Directory' field to most things
2 participants