Skip to content

Conversation

@asukaminato0721
Copy link
Contributor

@asukaminato0721 asukaminato0721 commented Mar 5, 2024

  • use ...args
  • add missing @return

Resolves p5-types/p5.ts#123

Changes:

Screenshots of the change:

PR Checklist

- use `...args`
- add missing `@return`
@Qianqianye Qianqianye requested a review from limzykenneth March 9, 2024 01:40
@lindapaiste
Copy link
Contributor

The @return statements look good. So do the default arguments length = 1. I'm not understanding the benefit of switching from named arguments to ...args.

@limzykenneth
Copy link
Member

@lindapaiste There's a bit of a tradeoff here I think. Named arguments probably will make things slightly clearer but ...args have the benefit of being actual array that array prototype methods can be called on, ie. it saves the need to convert arguments to an actual array when required.

@lindapaiste
Copy link
Contributor

@lindapaiste There's a bit of a tradeoff here I think. Named arguments probably will make things slightly clearer but ...args have the benefit of being actual array that array prototype methods can be called on, ie. it saves the need to convert arguments to an actual array when required.

It looks like only 3 of these methods do the [...arguments] casting and the rest don't.

Maybe I'd see the benefit more if the intermediate variable vectorComponents was removed, since it no longer serves any purpose when it's the same variable as args (const vectorComponents = args;). You can now call args.every(element => directly.

@asukaminato0721
Copy link
Contributor Author

the benefit of switching from named arguments to ...args.

because the origin x, y, z is kind of misleading. It's not something like Promise(resolve, reject), in which resolve is always resolve. The x can be not just "x" , but an array, which is called overload.

Besides, according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments#description

Note: In modern code, rest parameters should be preferred.

@Ethan-C-Honzik
Copy link

Wondering what the status is on this? Some of my code is currently littered with // @ts-ignore for every vector operation so the return types would be nice.

@limzykenneth limzykenneth merged commit df9f948 into processing:main Apr 12, 2024
@limzykenneth
Copy link
Member

Looks good. Thanks!

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.

issue from email

4 participants