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

Linter ControlStatementSpacing raise incorrect = #101

Closed
itsmechlark opened this issue May 30, 2019 · 6 comments
Closed

Linter ControlStatementSpacing raise incorrect = #101

itsmechlark opened this issue May 30, 2019 · 6 comments
Labels

Comments

@itsmechlark
Copy link

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)
@sds
Copy link
Owner

sds commented Jun 5, 2019

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

@sds sds added the bug label Jun 5, 2019
@mmzoo
Copy link
Contributor

mmzoo commented Jun 5, 2019

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.)

@sds
Copy link
Owner

sds commented Jun 5, 2019

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).

@chris-hewitt
Copy link
Contributor

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.

@rotelstift
Copy link
Contributor

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.

sds pushed a commit that referenced this issue Jan 10, 2024
This commit is to resolve issue
#101 .

I'm not familiar with OSS, so I apologize if I'm being impolite.

I'm also using machine translation for English.
@sds
Copy link
Owner

sds commented Jan 15, 2024

Implemented in #164.

@sds sds closed this as completed Jan 15, 2024
MegaDev007 added a commit to MegaDev007/slim-lint that referenced this issue Aug 3, 2024
This commit is to resolve issue
sds/slim-lint#101 .

I'm not familiar with OSS, so I apologize if I'm being impolite.

I'm also using machine translation for English.
masyuko0222 added a commit to masyuko0222/BookRIn that referenced this issue Dec 10, 2024
sds/slim-lint#101
・上記(解決してるっぽいけど...)と同じように、`\`で改行している影響でlintに引っかかっているかも
・「=前後にスペースを入れろ」という指摘だがちゃんと修正しても同じ指摘が出続けてしまう
・いつか直すとして一旦ignore
masyuko0222 added a commit to masyuko0222/BookRIn that referenced this issue Dec 10, 2024
lint対応

・sds/slim-lint#101
・上記(解決してるっぽいけど...)と同じように、`\`で改行している影響でlintに引っかかっているかも
・「=前後にスペースを入れろ」という指摘だがちゃんと修正しても同じ指摘が出続けてしまう
・いつか直すとして一旦ignore    ・sds/slim-lint#101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants