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

Improve handling of comment characters in ERB and EJS #19279

merged 7 commits into from May 8, 2019


Copy link

@jasonrudolph jasonrudolph commented May 7, 2019

Identify the Bug

atom/language-ruby#263 identifies a bug where Ruby line comments can break ERB syntax highlighting. This pull request is the first step in resolving that bug. Once this pull request is merged, we'll need to make a small change in language-html to use the new abilities introduced in this pull request. Once this pull request and atom/language-html#227 are both merged, it will resolve the bug.

Description of the Change

In languages like ERB and EJS, there is an implicit newline at the end of each directive. For example, in the ERB below, even though it only involves one line, there is an implicit newline between the two ERB directives, thus resulting in two separate Ruby statements.

<% def some_method %><% end %>

In Ruby, this becomes:

def some_method

The implicit newline is essential, because def some_method end isn't valid Ruby.

Prior to this pull request, these implicit newlines worked correctly in most cases, but didn't work correctly for comments. When an ERB directive contained a Ruby-style comment (or a JavaScript style comment in EJS), all subsequent ERB directives were treated as a continuation of that comment. (See the before and after screenshots below.)

In this pull request, we add the ability for injection points to specify whether an implicit newline should be applied between each directive. For example, 11cdac739271eb027c9b6b8c236d4c4e0cfef25f makes use of this new option to fix the bug identified in #263:

atom.grammars.addInjectionPoint('text.html.erb', {
  type: 'template',
  language (node) { return 'ruby' },
  content (node) { return node.descendantsOfType('code') },
  newlinesBetween: true

Before and After

The screenshots below use following code sample:

<%# some erb-style comment %>

<% puts 'some ruby code' %>

<% puts 'some ruby code followed by a comment' # some ruby-style-comment %>

<% puts 'now everything is treated as a continuation of the previous comment' %>

Screen Shot 2019-05-08 at 11 07 55 AM


Screen Shot 2019-05-08 at 11 08 37 AM

Alternate Designs

See the "Possible Solutions" section in tree-sitter/tree-sitter#327.

Possible Drawbacks

The approach implemented in this pull request only works if a newline character exists between the two directives; however, we expect this to cover a majority of cases. Future enhancements in Tree-sitter may provide a way to remove this limitation (tree-sitter/tree-sitter#327).

Verification Process

  • Open JavaScript file and verify proper syntax highlighting of interspersed functions and comments
  • Open Ruby file and verify proper syntax highlighting of interspersed functions and comments
  • Open HTML+ERB file and verify proper syntax highlighting of interspersed ERB comment directives, ERB directives with Ruby comments, and ERB directives without Ruby comments
  • Open HTML+EJS file and verify proper syntax highlighting of interspersed EJS comment directives, EJS directives with JavaScript comments, and EJS directives without JavaScript comments

Release Notes

N/A: See #19291.

@jasonrudolph jasonrudolph marked this pull request as ready for review May 8, 2019
@jasonrudolph jasonrudolph requested a review from maxbrunsfeld May 8, 2019
@jasonrudolph jasonrudolph self-assigned this May 8, 2019
@jasonrudolph jasonrudolph merged commit ecbdb3d into master May 8, 2019
2 checks passed
@jasonrudolph jasonrudolph deleted the mb-newlines-between-injection-points branch May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants