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

MBS-13496: Support attributes with spaces in link phrase interpolation #3186

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

Fix MBS-13496

Problem

Using the new "a cappella" attribute is breaking link phrase interpolation, because spaces are not accepted as part of the interpolated "variable name". We already had several attributes with spaces in their name though, we just had gotten lucky we were not using any of them in link phrases until now.

Solution

Just adding spaces to the allowed characters doesn't seem to cause any new issues, so it seems good enough for our needs.

Testing

Added one test with spaces to the expand2 tests.

Further action

  • Rename the attribute to "a cappella" and update the link phrases of the relationship.
  • Check if the Weblate checks for interpolation need updating to support this change.

@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label Feb 26, 2024
@@ -46,6 +46,7 @@ test('expand2', function (t) {
);
expandText('An {apple_fruit}', null, 'An {apple_fruit}');
expandText('An {apple_fruit}', {apple_fruit: 'apple'}, 'An apple');
expandText('An {apple fruit}', {'apple fruit': 'apple'}, 'An apple');
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to have two more small tests for condSubstStart and linkSubstStart. :)

Also, I think MusicBrainz::Server::Translation::expand already supports this, but perhaps add a small test for the Perl side, too? There are some existing ones in t/lib/t/MusicBrainz/Server/Translation.pm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I understand correctly that condSubstStart would be used in tests such as expandText('{a:%{b:%|}%|}', {a: '0', b: ''}, '00');? And is linkSubstStart tested in tests like this?

  expandHtml(
    'A {apple_fruit|<strong>{prefix} APPLE!</strong>}',
    {apple_fruit: 'http://www.apple.com', prefix: 'dang'},
    'A <a href="http://www.apple.com"><strong>dang APPLE!</strong></a>',
  );

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a couple tests to the Perl file :)

Using the new "a cappella" attribute was breaking link phrase
interpolation, because spaces were not accepted as part of the
interpolated "variable name".
We already had several attributes with spaces in their name
though, we just had gotten lucky we were not using any of them
in link phrases until now.

Just adding spaces to the allowed characters doesn't seem to cause
any new issues, so it seems good enough for our needs.
@yvanzo
Copy link
Contributor

yvanzo commented Mar 5, 2024

@brainzbot please retest this.

@@ -206,7 +206,7 @@ export const createTextContentParser = <T, V>(
return mapValue(text);
};

const varSubst = /^\{([0-9A-z_]+)\}/;
const varSubst = /^\{([0-9A-z_ ]+)\}/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Beginning/ending spaces should probably not be allowed.

Copy link
Member Author

@reosarevok reosarevok Mar 5, 2024

Choose a reason for hiding this comment

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

Is there a reason why they shouldn't? I mean, I don't think we're going to use them, but what's the damage? We're also probably not going to use ending underscores but we allow them.

Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent using them indeed. Damage is broken interpolation and translation. Same for underscores.

Copy link
Member Author

Choose a reason for hiding this comment

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

But that would only be a problem if we actually did add spaces at the beginning or end of variables, or a translator messed up (which is already the case even without the spaces) :) That said, if the regex can be easily changed to your preference, I have nothing against committing a suggestion for it, I just don't think we should complicate it too much otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I don’t think that allowing spaces in variable names is a good approach at all, see below.

root/static/scripts/common/i18n/expand2.js Show resolved Hide resolved
@yvanzo
Copy link
Contributor

yvanzo commented Mar 5, 2024

Check if the Weblate checks for interpolation need updating to support this change.

Yes, at https://github.com/metabrainz/musicbrainz-weblate-checks/blob/main/weblate_musicbrainz_format_check/checks.py#L17 and tests too.

A more durable implementation would be to use a slug of attribute name for variable name.

In either case, the examples at https://wiki.musicbrainz.org/MusicBrainz_Server/Internationalization#Variables have to be updated accordingly.

@reosarevok
Copy link
Member Author

reosarevok commented Mar 8, 2024

A more durable implementation would be to use a slug of attribute name for variable name.

Do you mean that we should convert all attributes to what, replace spaces by hyphens and then check the variables? I'm not against that if it feels better, I guess, I just thought it might be more confusing but maybe not?

@yvanzo
Copy link
Contributor

yvanzo commented Mar 8, 2024

replace spaces by hyphens

By underscores, to follow the convention already in use for variables in the MusicBrainz Server code.

I just thought it might be more confusing

On the contrary, underscores make variable names more easily recognized as not translatable code by translators.

@reosarevok
Copy link
Member Author

Hmm. Well, we already support underscores, meaning it could theoretically clash between a name with a space and one with underscores, but we could probably make sure we don't mess that up at least :)

@yvanzo yvanzo marked this pull request as draft March 12, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants