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

Use default args now that node 6 is required. #1633

Merged
merged 1 commit into from
Feb 7, 2017
Merged

Use default args now that node 6 is required. #1633

merged 1 commit into from
Feb 7, 2017

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Feb 3, 2017

There's also polyfillNode4Support which I can't quite look into now. If the name is wrong, the function should be renamed otherwise cleaned up.

@brendankenny
Copy link
Member

maybe we should do one big pass for places where we can drop the || dance and use default parameters? A quick search didn't turn up that many, but enough that a PR for each is going to get pretty noisy :)

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Feb 3, 2017

Agreed, but I just looked for node comments and saw this one and the polyfillNode4Support function.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Feb 3, 2017

@brendankenny: please check now. I enabled ESLint's no-param-reassign rule locally to quickly see the errors.

@XhmikosR XhmikosR changed the title printer.ts: use default args now that node 6 is required. Use default args now that node 6 is required. Feb 3, 2017
@brendankenny
Copy link
Member

@XhmikosR I fixed the merge conflict in the github UI and immediately regretted it :) Do you want to back that out, fix the merge conflict and I can land?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for tracking all these down!

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Feb 7, 2017

@brendankenny: rebased, check now.

@brendankenny brendankenny merged commit c88ea11 into GoogleChrome:master Feb 7, 2017
@brendankenny
Copy link
Member

thanks again!

@XhmikosR XhmikosR deleted the node6-cleanup branch February 7, 2017 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants