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

Lint/Syntax: unexpected token tPIPE after upgrade to 0.46.0 #411

Closed
pcothenet opened this issue Jun 29, 2023 · 6 comments · Fixed by #415
Closed

Lint/Syntax: unexpected token tPIPE after upgrade to 0.46.0 #411

pcothenet opened this issue Jun 29, 2023 · 6 comments · Fixed by #415
Labels

Comments

@pcothenet
Copy link

I assume this has to do with the change of Rubocop linter with 0.46.0 (https://github.com/sds/haml-lint/releases/tag/v0.46.0).

We have a lot of multi-line in our haml. Things that do not fit well in Ruby, like locals for partials, or long data attributes:

Screenshot 2023-06-28 at 5 19 08 PM

The syntax with |, while ugly (and breaking the VSCode highlighting) is valid .haml. However, the upgrade to 0.46.0 starts throwing errors for all of those:
Screenshot 2023-06-28 at 5 20 27 PM

I assume this has to do with the explanation here: #407 (comment)

haml-lint basically converts the HAML to valid ruby, runs rubocop, and then transfers the fixes

Any advice since Lint/Syntax cannot be disabled?

@MaxLap
Copy link
Contributor

MaxLap commented Jun 29, 2023

Yikes, I assumed that using pipes like this would be rare.

I just fixed a bug related to them to at least avoid totally failing like this, but I don't think it will fix this.

I will investigate to, at the very least, ignore those lines instead of failing. Actually processing them is quite trickier, so while I would like to, it might take a while before I get to that.

If I may make a suggestion, HAML allows changing line when they end in a comma. So your example could be rewritten as (I cut some parts for the example since I want to retype the whole text from your image) :

= render(partial: "invite....",
         locals: { form_for: aaa,
                   parent_organisation: bbb }

No pipes needed! And this is supported as-is by haml-lint right now (the linting/auto-correct will work with it). But I understand changing everything isn't possible. I'll let you know when it's fixed to at least ignore those case.

@pcothenet
Copy link
Author

OK, let me give it a try!

@pcothenet
Copy link
Author

This does seem to work on my example. Let me check a couple other use cases. I'll share with my team to see if we want to transition. Then I only have 480 files to correct 😁

@MaxLap
Copy link
Contributor

MaxLap commented Jun 29, 2023

Haha, wow, 480 files. I'd suggest waiting a bit before getting to that (maybe stay on the previous haml-lint version). If we do somehow mange to handle the pipes, it's quite possible that cases ending with a comma then a pipe would automatically switch to comma only as part of auto-correction.

I'll have to investigate some more how complex of a change than is.

@pcothenet
Copy link
Author

We weren't happy with our use of pipes, so got rid of them, but they could be useful to others.

A couple other things I've noticed anecdotally while refactoring:

  • A couple false positives in Lint/UnusedBlockArgument for large multi-line blocks
  • Also seemed to be an issue with \ to indicate a multi-line string (also valid Ruby)
  • A couple other false positives (undefined method [] for nil:NilClass) that seemed to be related to multi-line formatting

@MaxLap
Copy link
Contributor

MaxLap commented Jun 30, 2023

Thanks for the feedback. I'm working on the pipes support but a little busy. Im expecting to get it done this weekend.

Ideally, if you you provide an example to reproduce the first two cases, it would be helpful. I can try messing with backslashes but starting from an example saves time.

For the last point, its already fixed but hasn't been released in a new version yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants