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

ERB: Fix greedy comment matching #902

Merged
merged 2 commits into from
Jun 19, 2018
Merged

ERB: Fix greedy comment matching #902

merged 2 commits into from
Jun 19, 2018

Conversation

ananace
Copy link
Contributor

@ananace ananace commented Apr 25, 2018 •
edited
Loading

This little commit fixes the excessively greedy comment matching in ERB files, comments are after all constrained to the scope of the ERB tag in which they begin.

dblessing
dblessing reviewed Jun 13, 2018
View reviewed changes
Copy link
Collaborator

@dblessing dblessing left a comment

Choose a reason for hiding this comment

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

Thanks @ananace. Can you share an example of where it was overly greedy prior to this change? If you could add such an example to spec/visual/erb that would be really helpful.

@dblessing dblessing added the author-action The PR has been reviewed but action by the author is needed label Jun 13, 2018
@ananace
Copy link
Contributor Author

ananace commented Jun 14, 2018

And examples of the given example when rendered;

Before:
screenshot_20180614_093703

After:
screenshot_20180614_093722

@dblessing dblessing merged commit b1269e6 into rouge-ruby:master Jun 19, 2018
@dblessing
Copy link
Collaborator

Thanks @ananace

@ananace ananace deleted the fix-erb-comment branch August 16, 2018 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblessing dblessing dblessing left review comments

Assignees
No one assigned
Labels
author-action The PR has been reviewed but action by the author is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ananace @dblessing