Skip to content
This repository was archived by the owner on Jul 27, 2024. It is now read-only.

Ignore snippets in UndefinedObject#160

Merged
macournoyer merged 3 commits into
masterfrom
ignore-undefined-objects-in-snippets
Feb 17, 2021
Merged

Ignore snippets in UndefinedObject#160
macournoyer merged 3 commits into
masterfrom
ignore-undefined-objects-in-snippets

Conversation

@macournoyer
Copy link
Copy Markdown
Contributor

See #134 for a previous attempt at a fix. But we ended up recommending to use default on all optional parameters. But this is not a good solution.

See context in: https://shopify.slack.com/archives/C01F4ASU4G7/p1613572406096700?thread_ts=1613513255.092400&cid=C01F4ASU4G7

Can be turned back on w/ ignore_snippets: false

Fixes #159

Can be turned back on w/ `ignore_snippets: false`

Fixes #159
Comment thread config/default.yml Outdated

UndefinedObject:
enabled: true
ignore_snippets: true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the opposite? default to false and can be turned on?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont_ignore_snippets: false? :p

Good point! renaming

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, would it make more sense to name it exclude_snippets: true ?

To mimic the other one we already have: https://github.com/Shopify/theme-check/blob/master/config/default.yml#L33

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm saying this should be opt-in in Dawn rather than by default to everyone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I don't think this will roll. We want to use the same things internally that we'll promote externally.

So maybe this shouldn't even be an option? Always ignore the snippets.

Copy link
Copy Markdown
Contributor

@charlespwd charlespwd Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but what's the difference between a typo and an optional parameter in this case? Should you be flagged if you use a typo'ed argument?

This would turn off the rule by default in snippet files always. Seems like if I don't know anything about theme development, I would expect the UndefinedObject rule to flag me in that file if I'm using a typo'ed argument.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree that we should. But I can't think of a way to allow optional parameters in snippets, and check for this at the same time.

I think we had two choices:

  1. Force users to declare optional parameters (using | default: ...)
  2. Stop checking snippets for this, because snippets are not

I wished we could stick w/ 1, but I agree it does make the code more complicated, as mentioned Slack.

And we're going w/ 2, because "we don't want to encourage the use of snippets as functions" anyways.

Maybe there's another solution, but I don't see it atm.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright alright :p

Copy link
Copy Markdown
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ I echo all of the smart things you and @charlespwd have discussed 😛

@macournoyer macournoyer merged commit dd632cb into master Feb 17, 2021
@macournoyer macournoyer deleted the ignore-undefined-objects-in-snippets branch February 17, 2021 20:08
@macournoyer macournoyer temporarily deployed to rubygems February 17, 2021 20:14 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable running UndefinedObject in snippets

3 participants