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

False positive with Layout/SpaceInsideParens for multi-line attributes

Open gravitystorm opened this issue 1 year ago • 2 comments

I'm getting rubocop warnings regarding Layout/SpaceInsideParens, despite there being no parentheses on the indicated lines. I've narrowed this down to a small testcase, please see below.

I think this is a regression related to #487, since everything works on 0.55.0 but started failing on 0.56.0

$ cat foo.html.haml 
%foo{ attr: 'bar' }
%foo{ attr: 'bar',
      attr2: 'baz' }
bundle exec haml-lint --internal-debug foo.html.haml 
------ Extracted ruby from foo.html.haml:
haml_lint_tag_1
haml_lint_marker_2
WWWWW(attr: 'bar')
haml_lint_marker_4
haml_lint_tag_5
haml_lint_marker_6
WWWW( attr: 'bar',
      attr2: 'baz' )
haml_lint_marker_9
------
------ Raw lints found by RuboCop in foo.html.haml
C:  7:  6: Layout/SpaceInsideParens: Space inside parentheses detected.
C:  8: 19: Layout/SpaceInsideParens: Space inside parentheses detected.
------
foo.html.haml:7 [W] RuboCop: Layout/SpaceInsideParens: Space inside parentheses detected.
foo.html.haml:8 [W] RuboCop: Layout/SpaceInsideParens: Space inside parentheses detected.

1 file inspected, 2 lints detected

I can see that the spaces for the multi-line attributes are being preserved when converting to the internal representation which uses parentheses, which is then triggering the rubocop warning.

gravitystorm avatar Feb 06 '24 12:02 gravitystorm

Nice investigation!

But I don't know what to do... The whitespaces within the { used to be just removed, which lead some errors as #487. But this means that now, the system detects when the { has spaces after it.

The message from RuboCop is also kinda garbage since as you say, there is no paren visually.

Finally, control over that spacing feels like it should be part of haml-lint (a space around tag attributes lint) than a RuboCop cop, but since the goal is to generate an exact replica of the HAML's ruby for RuboCop to do its thing... This isn't so easy.

Side note: HAML examples use no spaces. So I would guess this is the "recommended" format.

I guess I could have a special case for when there are only spaces before the first attribute (no newlines), so that i remove them (and add as many W before the paren to keep things aligned), so that overall the code stays the same, but the paren is always touching the first attribute.

I'm guessing would I need to do something similar with the closing brace, remove space if no new-lines...

If I was to do that, then SpaceInsideParens configured for a single space would start failing... But I guess it was already failing before I fixed #487, so that's probably not an issue.

@sds Any thoughts on this?

MaxLap avatar Feb 06 '24 13:02 MaxLap

No strong opinion from me. Easier to provide input on a PR 🙂

Could also just disable that particular cop for HAML files, if a bandaid fix is desired.

sds avatar Feb 27 '24 04:02 sds

A potential workaround to this is to move the attributes and closing paren to their own lines and intent them appropriately. Modifying @gravitystorm's original example:

$ cat foo.html.haml 
%foo{ attr: 'bar' }
%foo{
  attr: 'bar',
  attr2: 'baz'
}

Results:

bundle exec haml-lint --internal-debug foo.html.haml 
------ Extracted ruby from foo.html.haml:
haml_lint_tag_1
haml_lint_marker_2
WWWWW(attr: 'bar')
haml_lint_marker_4
haml_lint_tag_5
haml_lint_marker_6
WWWW(
  attr: 'bar',
  attr2: 'baz'
)
haml_lint_marker_11
------
------ No lints found by RuboCop in foo.html.haml

1 file inspected, 0 lints detected

Granted this might not be to everyone's preference, personally I don't think it is too bad a compromise for people looking to upgrade.

akamike avatar Apr 18 '24 13:04 akamike

The other option is to disable that cop for haml files. Adding this to your .rubocop.yml should do the trick.

Layout/SpaceInsideParens:
  Exclude:
    - "**/*.haml"

Sorry but this problem feels like playing Whac-A-Mole with a bug. Anything I do feels like it willlead to another regression or that I will need to alter RuboCop. I don't have the will (nor the time) to tackle this, especially with the possible workaround of turning off that cop.

I should have said that earlier. Sorry I didn't.

MaxLap avatar Apr 18 '24 14:04 MaxLap

Going to close since a workaround has been provided. Thanks!

sds avatar Jun 24 '24 19:06 sds

Just to check I understand correctly, the recommended workaround is disable Layout/SpaceInsideParens for *.haml?

tisba avatar Jun 25 '24 08:06 tisba

Yes, that seems like the most straightforward way to work around the issue.

sds avatar Jun 25 '24 21:06 sds