Linting produces false positives with .erb files

[ricardograca]

When html-linter tries to lint erb files, it incorrectly complains about <% and <%=.

It will also complain that the file should have a doctype, which obviously isn't needed in an erb view or partial.

[Arcanemagus]

If this linter doesn't properly extract the text, it probably shouldn't be run on any script file...

[ricardograca]

Good point. I'll change the title and description.

[johnwebbcole]

You can turn this check off in an .htmlhintrc file.

[ricardograca]

Turning it off won't help me if I have a special character outside of the ERB code that should produce an error.

[johnwebbcole]

Have you tried the linter-erb plugin?

[ricardograca]

No, but I will :) Thanks for the tip. So, this linter is really only meant for files that only contain html right?

[johnwebbcole]

I use it regularly with angular, and handlebars tags. Others work ok too, but tags that are legit for the template are invalid html, so I usually disable that rule for those projects. It's nice that you can put a .htmlhintrc file and just disable it for one directory. While you loose that rule, the others still work.

If there is a better one available, you should definitely use it, and linter-htmhint should not interfere with it. so please let me know if the liter-erb plugin does the job and I'll remove the erb files from the list of files. I don't want to remove it if linter-htmlhint does a better job (but I doubt it is).

Of course you may find that running both is desirable, after disabling the rules that cause false positives in linter-htmlhint.

Let me know.

[ricardograca]

Right, so I already had linter-erb enabled but I wasn't seeing any output out of it. Went over to the repo and it seems that it isn't working due to changes in linter: AtomLinter/linter-erb#11

[johnwebbcole]

Ok, well I think we should leave erb files in, at least until linter-erb is working again. That way you have some listing, even if you can't use the special character rule.

If you would, please open another ticket when you feel linter-erb is working to the point it's better than linter-htmlhint and we can remove that one form the scope list.

[ricardograca]

So, here's the thing. With this package active I get a ton of incorrect warnings in .erb files as mentioned above. If I disable linter-htmlhint then linter-erb will correctly report errors in the ruby parts of templates, but on html parts it doesn't complain about anything.

So, what should the expected behavior be in this case? Maybe disable the .erb scope in this package? Would that still provide html linting for the html parts in an .erb template?

[johnwebbcole]

If linter-erb works, then linter-htmlhint should not do the linting, IMO. Any issues in should then be addressed to linter-erb.

It's possible to have both running, but you would want to place a .htmlhint.rc file in the directory with your .erb templates and disable the offending rules.

[ricardograca]

I don't understand why I have to provide custom rules. Is it expected behavior that by default if I have both linters installed I get a lot of incorrect errors reported?

Is the linting done based on the language scope of each code block or is it per file?

[johnwebbcole]

It's per file.

[ricardograca]

Right. I don't know much about how atom-linter works, but according to this bug report it seems to be possible for a single file to pass linting duties to other linters when there are multiple scopes involved.

What needs to happen so that I can have both linter-htmlhint and linter-erb activated and get correct results for the html and erb sections?

[amingilani]

+1 I have both htmlhint and linter-erb, and htmlhint drives me crazy

[ricardograca]

@amingilani I feel your pain. On semi-good news I just became the maintainer of linter-erb so I hope to fix this situation soon. I'm just starting on learning how Atom package authoring works though, so it may take a while.

[amingilani]

@ricardograca wooohoooo! Okay, thank you!

[Arcanemagus]

@ricardograca The problem isn't with linter-erb, but with this package claiming to support ERB files when it can't.

If you want to see the reasoning on why it still lists those scopes you might want to read though #52.

Personally I feel this package should only list itself as compatible with pure HTML as that is all the base linter actually understands, but @johnwebbcole disagrees, and as there is some utility in having it run on other scopes I haven't pressed the matter.

Perhaps it's time to revisit this discussion, what do people think about only listing HTML by default, but allowing entering of a list of custom additional scopes? That way anyone can have this run on any scope they want, but they should know then that what they are doing isn't really supported and they may have to do some configuration to work with it (such as ignoring errors that don't mean anything in the "extra" scopes).

[ricardograca]

I know that the problem isn't on linter-erb's side, but I was going to provide a PR to this package to remove ERB support here.

I totally agree that this package should only lint html. It's the html linter after all, not the html/erb/handlebars/whatevever linter, so it's a bit unexpected that it also lints other file formats. Besides unexpected it also causes a lot of issues as has been proved by the number of people complaining about this package interfering with linting in other scopes.

I also agree with the optional additional scopes to those that need that kind of support, but it should not be the default.

[johnwebbcole]

The last time we were asked about erb support, the asker decided that the flawed htmlhint was better than nothing or the erb linter at the time. I agree that if a linter supersedes htmlhint, it should be removed, but I use htmlhint with angular and other templates just fine. I simply add a .htmlhintrc file that turns off annoying false positives and the htmlhint site makes it simple to create the file.

When I had a limited set of scopes (the ones I used), I got request after request to add more, and erb was one of those specifically asked for. Removing scopes that don't have good alternatives is going to run into the same issue, people asking for them to be added back. Some way to select which scopes to use in the Atom config page would be a good solution however.

[amingilani]

@johnwebbcole I encourage you to do the inverse by explicitly defining htmlhint only for html files. Just like you wouldn't add htmlhint to .php files, you shouldn't with .erb.

If you add a line in the README with instructions on how to extend htmlhint to non html files, that'd be great for people who want it there.

[ricardograca]

@johnwebbcole Personally I think that linter packages should just work without any workarounds needed, and you mentioned that even in your use case you have to provide a workaround for the "annoying false positives". I'm sure no one likes to be annoyed when trying to get work done, so that's why defaulting to html only with the option to provide additional scopes seems like a reasonable compromise.

I can provide a PR to disable all scopes except html, but I have no idea how to add something to the settings page.

Also, ideally it should be possible to call the html linter for the html portions of other language's templates, but I also have no idea how that's done. I think this would be a great step towards ensuring that no one would need to ask the html linter to include support for templating languages.

Finally for statistics here are the issues requesting templating languages to be included:

#3

And here are the issues caused by supporting templating languages:

#41, #42, #43, #44, #47, #51, #73, #82, #94

There are others mentioning the "unexpected end of input" error, but it's not clear if it's due to being used on a templating language or not. I think it's pretty clear that there would be a lot less issues if this package just linted html and nothing else.

[Arcanemagus]

Re: calling the linter on "the HTML portions"

You would need some sort of extractor for every extra language that you wanted to support in this manner, as that doesn't exist for most of them this isn't really a viable method unfortunately.

[ricardograca]

Ok, but specifically in the case of ERB files. When using linter-erb on those files no checking whatsoever is performed on the html parts. That sucks. Including an html linter as well in that package would be awkward and result in duplication of efforts when there is already an html linter, so that's not an option.

When you say that an extractor would be required for every language, does that mean the extractor would be required in the template language linters or in the html linter package?

Isn't it possible for a package to pass a string (or buffer or whatever) to another package? It would be super easy to remove all template specific code and leave only the html part of a file from the linter-erb side.

[Arcanemagus]

This package would need to have the extractor, as it would be the one needing the HTML contents.

If ERB provides a simple way to get the HTML that's great... but I'm not sure how you would run linter-htmlhint on it, possibly using the Indie API to linter. The system was never really meant to have linters running each other.

[ricardograca]

I see. So, the final question is: shouldn't the scopes change according to the part of the file being analyzed instead of having a single scope per file? If I was designing a linting API that's what I'd aim for. That way each specific linter would deal with the parts of files that it knows, and wouldn't have to pass anything to other linters. That is, the base linter package would handle all that.

[Arcanemagus]

The scope does change based on where you are in a file, assuming you are dealing with a file that can change scopes. The issue is that those should be marked as embedded source. This was a major problem we had with the default Markdown language package in Atom actually, as it was marking all embedded code blocks with the full scope. So for example if you did something like this:

var foo = 42;

And had your cursor inside that block of embedded JavaScript the language-gfm package was claiming that the cursor was within a source.js scope, instead of the proper source.embedded.js. This meant we had all sorts of issues with linters running on Markdown files and giving all sorts of errors and false positives since they thought they were running on a raw source file of the language they understood and all the Markdown stuff confused them.

Some languages (I believe ERB is like this) simply claim the entire file is their language, even if they are a template language.

An extractor takes those blocks of "other" code and brings them out into a manner that the linter understands, then when it's done linting translates things like lines and columns back to where they should be in the original file.

That's an interesting idea about having linter itself be an extractor, but I don't think it's that viable right now as Atom's scoping system works well enough for the simple use cases so far, but it needs a lot of work to be a fully viable enough scope system for something like that. There isn't even any documentation on what scopes should even be called yet 😛. You also would need to deal with the complexity of the position translation.

[ricardograca]

I suspected it wouldn't be easy, but I think it's important to get there eventually. Documenting already used scope names also seems important and I've seen some discussion about that in other issues. Seems like that part is still a bit in its early stages.

Well, I'll stop complaining for now, but will come back to it later when it starts bothering me :)