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.

Sign up for GitHub

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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions root/static/scripts/common/i18n/expand2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

export const createVarSubstParser = <T, V>(
argFilter: (V) => T,
): Parser<T | string | NO_MATCH, V> => saveMatch(
Expand All @@ -225,7 +225,7 @@ export const createVarSubstParser = <T, V>(
export const parseStringVarSubst: Parser<string | NO_MATCH, mixed> =
createVarSubstParser<string, mixed>(getString);

const condSubstStart = /^\{([0-9A-z_]+):/;
const condSubstStart = /^\{([0-9A-z_ ]+):/;
yvanzo marked this conversation as resolved.
Show resolved Hide resolved
const verticalPipe = /^\|/;
export const substEnd: RegExp = /^}/;
export const createCondSubstParser = <T, V: Expand2ReactInput>(
Expand Down
2 changes: 1 addition & 1 deletion root/static/scripts/common/i18n/expand2react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type Output = Expand2ReactOutput;
const textContent = /^[^<>{}]+/;
const condSubstThenTextContent = /^[^<>{}|]+/;
const percentSign = /(%)/;
const linkSubstStart = /^\{([0-9A-z_]+)\|/;
const linkSubstStart = /^\{([0-9A-z_ ]+)\|/;
yvanzo marked this conversation as resolved.
Show resolved Hide resolved
const htmlTagStart = /^<(?=[a-z])/;
const htmlTagName = /^(a|abbr|br|code|em|h1|h2|h3|h4|h5|h6|hr|li|ol|p|span|strong|ul)(?=[\s\/>])/;
const htmlTagEnd = /^>/;
Expand Down
3 changes: 2 additions & 1 deletion root/static/scripts/tests/i18n/expand2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import expand2text, {
} from '../../common/i18n/expand2text.js';

test('expand2', function (t) {
t.plan(69);
t.plan(70);

let error = '';
const consoleError = console.error;
Expand Down Expand Up @@ -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 :)

expandText('A {number}', {number: 1}, 'A 1');
expandHtml('{null} value', {null: null}, ' value');
t.equal(error, '');
Expand Down
6 changes: 6 additions & 0 deletions t/lib/t/MusicBrainz/Server/Translation.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,15 @@ test 'Check _expand_link' => sub {
is(MusicBrainz::Server::Translation->instance->expand('An {apple_fruit}',
apple_fruit => 'apple'),
'An apple', 'Simple replacement');
is(MusicBrainz::Server::Translation->instance->expand('An {apple fruit}',
'apple fruit' => 'apple'),
'An apple', 'Simple replacement with spaces in key');
is(MusicBrainz::Server::Translation->instance->expand('An {apple_fruit|Apple}',
apple_fruit => 'http://www.apple.com'),
'An <a href="http://www.apple.com">Apple</a>', 'Replacement with links');
is(MusicBrainz::Server::Translation->instance->expand('An {apple fruit|Apple}',
'apple fruit' => 'http://www.apple.com'),
'An <a href="http://www.apple.com">Apple</a>', 'Replacement with links and spaces in key');
is(MusicBrainz::Server::Translation->instance->expand('A {apple_fruit|apple}',
apple_fruit => 'http://www.apple.com', apple => 'pear'),
'A <a href="http://www.apple.com">pear</a>', 'Replacement with link description evaluation');
Expand Down