Add an option to ignore objects with a given prefix in UndefinedObject

[macournoyer]

Fixes #133

[charlespwd]

• Is this an existing convention or a new feature?
• Should this be convered by something like disable-theme-check UndefinedObject:foo-bar?

[macournoyer]

It was suggested by @tyleralsbury in #133. It's the only way I could think of to support detecting undefined objects, or else we have to drop the whole check.

That sounds like a got idea for the comment. Do you know if eslint allows something similar?

[tyleralsbury]

As for whether it's a convention or something new - in this case, it's something new, though it's more or less based on established patterns like dangling underscores for private methods in other languages.

---

The comment idea is an interesting approach as well. In ESLint you have two options - you can either disable it for a line or for the entire file, which might be good for cases like this, so maybe:

disable-next-line-theme-check UndefinedObject:foo_bar

and

disable-theme-check UndefinedObject:foo_bar (at the top of the file)

To allow for both options.

Being able to do this for all of the things theme-check looks for would be a nice way of allowing the developer to control which rules they want to care about for a particular file or line to account for all cases that we haven't thought of - if the developer is getting a report from theme-check that they think is wrong, they can just disable it.

Ideally, this would also come with a "quick fix" solution in the VS Code plugin. When you have an issue like that in ESLint, you can click "quick fix" and one of the options is "add disable-eslint ... ` for that particular line.

ESLint docs for disabling rules: https://eslint.org/docs/2.13.1/user-guide/configuring#disabling-rules-with-inline-comments

[macournoyer]

@tyleralsbury good news 😄 => https://github.com/Shopify/theme-check#disable-checks-with-comments (not yet released on brew)

So the snippet template using optional variables would have this at the top:

{% comment %}
theme-check-disable UndefinedObject:optional_var1
theme-check-disable UndefinedObject:optional_var2
{% endcomment %}

Sounds good?

[tyleralsbury]

Yeah I think this is a really good solution - it accounts for this case and any other possible case that we didn't think of specifically. Very scalable. 👍

[tskorupa]

Most of the optional variables in templates could be assigned at the top unless they are passed in. I'm not sure if this is possible in liquid;

assign optional_variable = optional_variable || nil

This would avoid introducing a linter language to the templates.

[macournoyer]

Oh yeah that's really good @tskorupa !!

But can't use expression in assign, only filters I think. But since variables default to nil it could be:

{% assign optional = optional %}

It does look weird :/

[macournoyer]

This would work too, more explicit:

{% assign optional = optional | default: nil %}

[macournoyer]

I'm closing this in favor using {% assign optional = optional | default: nil %} to define optional vars.

I'll document in the ticket, but we might also need a way to educate users about this. Not sure the error message is the right place...

[tskorupa]

I'll document in the ticket, but we might also need a way to educate users about this. Not sure the error message is the right place...

Let me know if the error belongs to the snippet instead, I can modify it to say the likes of "undefined variable FOO, if it's an optional variable, you can add {% assign optional = optional | default: nil %} at the top of the snippet".

Even better, we can make that fix automatically if the developer wants it so.