-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰Expander: better support of backticks #26264
Conversation
@micajuine-ho can you take a look. Thank you |
@micajuine-ho I saw you recently added ownership of url-replacements. Do you think we should add expander to wg-analytics as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commenting to refresh owners bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trimming a string can be surprisingly expensive.
src/service/url-expander/expander.js
Outdated
builder = ''; | ||
// Collect any chars that may exist before backticks, eg FOO(a`b`) | ||
if (builder.trim().length) { | ||
results.push(builder.trim()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Store the trimmed value, don't re-trim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/service/url-expander/expander.js
Outdated
); | ||
builder = ''; | ||
// Collect any chars that may exist before backticks, eg FOO(a`b`) | ||
if (builder.trim().length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: a length check is unnecessary, compare to an empty string or just let if (str) push(str);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/service/url-expander/expander.js
Outdated
// create a new array for the next argument, and reset our string | ||
// builder. | ||
// We push any string built so far, create a new array for the next | ||
// argument, and reset our string builder. | ||
if (builder.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you be checking trimmed length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good catch. Done.
src/service/url-expander/expander.js
Outdated
if (nextArg) { | ||
results.push(nextArg); | ||
if (builder.trim().length) { | ||
results.push(builder.trim()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Good to know about the trimming, thanks. |
Previously the parser ignoring feature only worked when an argument was completely wrapped in backticks eg
FOO(abc, ` - `)
This allows backticks to be used anywhere inside of a macro to give raw input.
Follow up to #26247.