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

Add blacklist support to simple-unless rule #518

Merged

Conversation

mattbalmer
Copy link
Contributor

A project I'm working on has need of a blacklist feature in conjunction with the 'simple-unless' rule. We would like to allow most cases for {{unless}}, but prevent certain helpers from being used with them.

Code changes include adding that blacklist support, as well as modifying maxHelpers to treat a value of -1 as 'no limit'. This is useful because otherwise, to use the blacklist, one would need to specify an arbitrarily large number for maxHelpers (as it would raise errors on helpers not in the blacklist when the limit, defaulted to 0, was reached).

@bradcypert
Copy link

I can see how this is useful. Seems like a good feature to have.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review here! This seems like a nice addition to me, I left a small inline tweak and we also need a rebase.

Should be good to land after that.

@@ -142,6 +147,14 @@ module.exports = class LintSimpleUnless extends Rule {
this._logMessage(message, loc.line, loc.column, actual);
}

if (blacklist.length > 0 && blacklist.indexOf(param.path.original) > -1) { // blacklist.includes(param.path.original)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use .includes now (we dropped Node 4 support a little while back), mind swapping to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Also updated a similar usage in the whitelist check above.

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2019

@mattbalmer - 👋 think you might have time to circle back to this one? Again, I'm sorry I let it linger so long after you initially submitted it...

@mattbalmer mattbalmer force-pushed the feature/simple-unless-blacklist branch from 03f0644 to 7d66681 Compare June 20, 2019 01:04
@mattbalmer
Copy link
Contributor Author

Hey, no worries and sorry for my own slow response! Your suggestion has been included, and the styles updated to match the new linter rules. Let me know if you spot any other changes you'd like!
Thanks

@rwjblue rwjblue merged commit 56b944b into ember-template-lint:master Jun 20, 2019
@rwjblue
Copy link
Member

rwjblue commented Jun 20, 2019

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants