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 spread operator to get the arguments more safely #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cyan33
Copy link

@cyan33 cyan33 commented Mar 6, 2018

Hi @zertosh,

Don't know if this repo is still under maintainence. Anyway, I've noticed that it uses a, b, c, d, e as placeholders for the %s to be replaced. This could be easily improved by using the spread operator. It enhances the readability and is more safe in case the length of the placeholders is more than 5.

Would love to listen to your thought on this. 😄

@adamdicarlo
Copy link

I'd be concerned that the spread operator in the function signature would result in the argument spreading being executed on every call, not just failing ones. How about old-school use of arguments when the condition fails?

@Sceat
Copy link

Sceat commented May 8, 2020

@adamdicarlo i don't see how is that a problem

@adamdicarlo
Copy link

@Sceat Yeah, me neither, now (that was a while ago). At the time I was thinking it could be a performance issue. With eliminating these kinds of checks in production builds, it's no issue at all (maybe that wasn't standard when I commented? 🤔 ) ... even for debug builds I don't think it's nearly inefficient enough to be a problem.

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.

3 participants