-
Notifications
You must be signed in to change notification settings - Fork 93
External class resolve #140
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
Conversation
This looks fine, but any reason for only a single predicate over an object with separate predicates? Given the single-predicate option, I imagine almost all passed predicates will be of the form function(name, node) {
switch (name) {
case ...:
return ...;
case ...:
return ...;
// .. and so on
}
} So it might be simpler to pass an object whose keys are the allowed class names. |
My thinking was that we might want to manipulate the class name before comparing. For instance, the default in esquery is that it converts the class name to lowercase. We might not want that for all languages, or we might want to otherwise alter how a class name is interpreted. To that end, it seemed like a better choice to just pass the raw class name and let us decide on the other end how we'd prefer to handle it. |
@nzakas Sure, simple transforms on the class name seem like a good enough reason. |
Thanks so much! |
if (options && options.nodeTypeKey) return false; | ||
|
||
const name = selector.name.toLowerCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Reading the code it looks like the selector name doesn't change. Moving that call back into the closure leads to .toLowerCase()
being called repeatedly with the same value. That's work that can be avoided and I guess that was the reason it was hoisted outside of the function closure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably left from the multiple rounds of refactoring. 👍
This allows a
matchClass
function to be passed on theoptions
object that will override the default class matching behavior when present.I had to change the PEG.js grammar to allow arbitrary identifiers after the
:
. Everything still appears to work correctly, but do let me know if there is a better way to make this change.