Skip to content
This repository has been archived by the owner on Feb 24, 2022. It is now read-only.

Make rules extendable #50 #52

Merged
merged 1 commit into from
Aug 24, 2016
Merged

Conversation

jaredlockhart
Copy link
Collaborator

Fixes #50

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3430327 on JaredKerim-Mozilla:50 into 7e8b27f on mozilla:master.

metadata[metadataKey] = buildRuleset(metadataKey, metadataRule)(doc);
} else {
metadata[metadataKey] = getMetadata(doc, metadataRule);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, but I'm still trying to wrap my head around the shift from typeof metadataRule === 'function' to Array.isArray() and if the code is doing the same thing here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a weird difference. Where previously we were looking for typeof function because we had already applied buildRuleset and so each 'rule' was a callable. But now we've pushed that off, and so we're looking for arrays which are lists of rules, and then we wrap them in buildRuleset and apply them, or recurse deeper.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9669500 on JaredKerim-Mozilla:50 into 7e8b27f on mozilla:master.

@pdehaan
Copy link
Contributor

pdehaan commented Aug 24, 2016

R+ Looks good to me (with the one question/concern noted above).
Logic looks the same as @k88hudson's earlier PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b018ece on JaredKerim-Mozilla:50 into 7e8b27f on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0cd91e5 on JaredKerim-Mozilla:50 into 7e8b27f on mozilla:master.

@jaredlockhart jaredlockhart merged commit 9943dc9 into mozilla:master Aug 24, 2016
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.

None yet

3 participants