slim-lint icon indicating copy to clipboard operation
slim-lint copied to clipboard

Linter ControlStatementSpacing raise incorrect `=`

Open itsmechlark opened this issue 5 years ago • 3 comments

It's weird why ControlStatementSpacing linter keeps raising Please add a space before and after the = even I didn't see any isse.

div layout="column" layout-gt-sm=="row" ng-show="search_meta.total_pages | valPresent"
  .app-bg-color.md-primary.search-result flex=true
    | Search Results ({{ search_meta.total_count }} {{ search_meta.total_count > 1 ? 'documents' : 'document' }})
  .pagination.app-pagination[flex=true
    layout="row"
    layout-align="center center"
    layout-align-gt-sm="end center"
    ng-show="search_meta.total_pages > 1"]
      md-button.app-color-hover.app-primary[ng-href="{{ searchURL({page: 1}) }}"
        ng-disabled="search_meta.current_page == 1"
        aria-label=online_t(:first_page, scope: %i(document actions search))]
          md-icon.md-dark first_page
          md-tooltip md-direction="bottom" = online_t(:first_page, scope: %i(document actions search))
      md-button.app-color-hover.app-primary[ng-href="{{ searchURL({page: search_meta.prev_page}) }}"
        ng-disabled="search_meta.prev_page == null"
        aria-label=online_t(:prev_page, scope: %i(document actions search))]
          md-icon.md-dark keyboard_arrow_left
      md-button.app-primary[ng-repeat="page in searchNavPages(search_meta, search_meta.current_page)"
        ng-href="{{ searchURL({page: page}) }}"
        ng-class="{'app-bg-color': search_meta.current_page == page, \
        'app-color-hover': search_meta.current_page != page}"
        ng-bind="page"]
      md-button.app-color-hover.app-primary[ng-href="{{ searchURL({page: search_meta.next_page}) }}"
        ng-disabled="search_meta.next_page == null"
        aria-label=online_t(:next_page, scope: %i(document actions search))]
          md-icon.md-dark keyboard_arrow_right
          md-tooltip md-direction="bottom" = online_t(:next_page, scope: %i(document actions search))
      md-button.app-color-hover.app-primary[ng-href="{{ searchURL({page: search_meta.total_pages}) }}"
        ng-disabled="search_meta.current_page == search_meta.total_pages"
        aria-label=online_t(:last_page, scope: %i(document actions search))]
          md-icon.md-dark last_page
          md-tooltip md-direction="bottom" = online_t(:last_page, scope: %i(document actions search))

WARNINGS:

app/views/online/templates/_search_pagination.html.slim:26 [W] ControlStatementSpacing: Please add a space before and after the `=`
app/views/online/templates/_search_pagination.html.slim:31 [W] ControlStatementSpacing: Please add a space before and after the `=`

Dependencies:

slim_lint (0.17.0)
      rake (>= 10, < 13)
      rubocop (>= 0.50.0)
      slim (>= 3.0, < 5.0)
      sysexits (~> 1.1)

itsmechlark avatar May 30 '19 09:05 itsmechlark

@mmzoo you added this linter in #82. Can you investigate?

sds avatar Jun 05 '19 00:06 sds

Long time no see :D

In that code snippet there is one backslash \ that causes a weird behavior. Syntactically it is correct as per this (I don't think this minor bug affects it). Generally multi-line attributes seem not 100% robust, though.

Now, if I create a slim file with this content:

div class='one \
  two'
div = some_method

and run the linter over it in master, I get this error:

test.html.slim:2 [W] ControlStatementSpacing: Please add a space before and after the `=`

"That's good, I can reproduce the bug", I thought. But when I try to add a test for it, the test passes without error!

  context 'multi-line attributes' do
    let(:slim) { "div class='one \\n  two'\ndiv = some_method" }

    it { should_not report_lint }
  end
  # Or even like this
  context 'multi-line attributes' do
    let(:slim) {
<<-END
div class='one \
  two'
div = some_method
END
}

    it { should_not report_lint }
  end

I'm not sure what could cause this behavior. I have nothing in my .slim-lint.yml.

(Incidentally, in this example, the code line with the backslash ends on a comma, which also serves as a newline in the compiler. So, simply removing the backslash is a workaround to get the linter pass, without affecting the generated HTML.)

mmzoo avatar Jun 05 '19 07:06 mmzoo

Thanks for investigating!

Given there is a workaround, I'm inclined to leave this issue open for someone with enough interest to fix. Will happily merge a pull request fixing the issue (though I am at a loss as to why it would not be reproducible in the test environment).

sds avatar Jun 05 '19 21:06 sds

I think this error is due to slim-lint escaping a line break when it's preceded by a backslash. You can test this by adding a character after the backslash and seeing the error disappear.

To replicate it in a test, perhaps we could escape the slash: "div class='one \\\n two'\ndiv = some_method"

or used a non-escaped heredoc:

<<-'END'
div class='one \
  two'
div = some_method
END

Either way, I think underlying is a real error that causes slim-lint's line count to be off by one for every line that ends in a backslash. Hopefully I have time to familiarize myself with the repository and pursue this - but I'll leave this comment just in case I don't.

chris-hewitt avatar Dec 16 '23 01:12 chris-hewitt

I put out a PR for a code that solves this problem. I referred to @chris-hewitt 's comment for a test that reproduces the situation.

The problem seemed to be that https://github.com/sds/slim-lint/blob/main/lib/slim_lint/document.rb#L41 was creating source_line with a newline with or without a backslash. So, I put together a code to correct that.

This is my first time submitting a PR to OSS, so I apologize if I am rude. Also, I apologize if the English is difficult to understand due to machine translation.

rotelstift avatar Jan 06 '24 03:01 rotelstift

Implemented in #164.

sds avatar Jan 15 '24 02:01 sds