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

Jump to bottom

Extract regex to constant in HTML formatter #1904

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Extract regex to constant in HTML formatter #1904

merged 1 commit into from
Dec 14, 2022

Conversation

tancnle
Copy link
Collaborator

@tancnle tancnle commented Dec 13, 2022 •
edited
Loading

This helps to reduce multiple instantiations of escape regex variables in the HTML formatter. Also, add some stylish changes around the indentation.

This PR extracts partial changes proposed in #1670 by @robotmay.

I have verified that the change does not affect the HTML output via the visual tester.

Screenshot 2022-12-14 at 10 20 46 am

@tancnle @robotmay
Extract regex to constant in HTML formatter
2171b23 
This helps to reduce multiple instantiations of escape regex variables
in the HTML formatter. Also, add some stylish changes around the
indentation.

Co-authored-by: Robert May <rob@afternoonrobot.co.uk>
@tancnle
Copy link
Collaborator Author

tancnle commented Dec 13, 2022

@jneen Any thoughts on this PR since you have reviewed the related one? 🙏🏼

@jneen
Copy link
Member

jneen commented Dec 14, 2022

Do we have benchmarks for this? I'm a bit wary of perf related changes without actual benchmarks to back it up, since we've been sent on goose chases before.

@tancnle
Copy link
Collaborator Author

tancnle commented Dec 14, 2022

Thanks, @jneen.

Do we have benchmarks for this? I'm a bit wary of perf related changes without actual benchmarks to back it up, since we've been sent on goose chases before.

I don't think this is a perf-related change :) This PR does not include the gsub! implementation from the original PR.

@jneen
Copy link
Member

jneen commented Dec 14, 2022

Okay! It's fine by me then.

@jneen
Copy link
Member

jneen commented Dec 14, 2022

I should also say, gsub! should be safe, since the strings here come from regexp capture groups, which as far as I'm aware are created on match.

@jneen
Copy link
Member

jneen commented Dec 14, 2022

(FWIW we already mutate these strings to de-duplicate adjacent tokens of the same type in Lexer#continue_lex)

@tancnle tancnle merged commit ab49365 into rouge-ruby:master Dec 14, 2022
@tancnle tancnle deleted the maint.extract-escape-regex-html-formatter branch December 14, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tancnle @jneen