Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

support new Set(...) in ledger publisher ruleset #3545

Merged
merged 4 commits into from
Aug 30, 2016

Conversation

mrose17
Copy link
Member

@mrose17 mrose17 commented Aug 29, 2016

this accommodates the new longer exclusion list, cf., #3553

auditor: @BrendanEich

this accommodates the new longer exclusion list
@mrose17 mrose17 added this to the 0.11.6dev milestone Aug 29, 2016
@mrose17 mrose17 self-assigned this Aug 29, 2016
if (op1.name !== 'Set') throw new Error('only new Set(...) is allowed, not new ' + op1.name)
args = []
expr['arguments'].forEach((argument) => { args.push(traverse(argument)) })
return new Set(args[0])
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want return new Set(...args) for full generality? No point collecting the traversed arguments in args past the first one if you use only the first one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Set takes at most one argument, which is an array. if i were to pass args instead of args[0], then i would need to do use .apply() as well. i will make it clearer.

@bbondy
Copy link
Member

bbondy commented Aug 30, 2016

Is there an issue defined for this?

@bbondy
Copy link
Member

bbondy commented Aug 30, 2016

If not, please post an issue, mark 0.11.6 on the issue and then update the PR.
If so, please link up the issue.

The PR can be updated with:

git add -u
git commit --amend
git push -f

@mrose17
Copy link
Member Author

mrose17 commented Aug 30, 2016

i will create an issue.

@diracdeltas diracdeltas mentioned this pull request Aug 30, 2016
@BrendanEich BrendanEich merged commit f5496f0 into master Aug 30, 2016
@BrendanEich BrendanEich deleted the update-for-latest-publisher-ruleset branch August 30, 2016 14:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants