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

Default values of arguments && modernize some code #6807

Merged

Conversation

asukaminato0721
Copy link
Contributor

Resolves #6721

Changes:

use default values of arguments, fix some missing ...args, use some native apis.

Screenshots of the change:

PR Checklist

@asukaminato0721 asukaminato0721 changed the title Default values of arguments Default values of arguments && modernize some code Feb 12, 2024
@asukaminato0721 asukaminato0721 force-pushed the Default-values-of-arguments branch 2 times, most recently from 789327f to 9f0e406 Compare February 12, 2024 10:43
Copy link
Member

@limzykenneth limzykenneth left a comment

Choose a reason for hiding this comment

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

One comment inline and one more thing which is that there are some inconsistency in terms of space before parenthesis after the function key word, ie

function ()

// or

function()

For most of the rest of the code base, the consistent use is not having the space so I tend towards that. @nickmcintyre I can't find specific recommendation for this in the documentation style guide, do you have some thoughts on this?

src/math/p5.Vector.js Outdated Show resolved Hide resolved
@nickmcintyre
Copy link
Member

@limzykenneth consistency with no space function() seems like the way to go for now. I'd generally recommend against anonymous functions for documentation, but they're fine in source code.

Longer term, both Airbnb and StandardJS suggest space before parentheses for anonymous functions. Maybe we can revisit style guides on the road to 2.0.

@limzykenneth
Copy link
Member

I'll do a final review later today to see if private methods will work or not. After that I will merge this.

@limzykenneth limzykenneth merged commit 7a808ae into processing:main Feb 29, 2024
2 checks passed
@limzykenneth
Copy link
Member

The new syntax doesn't seem to work with browserify, we'll re-explore this as part of 2.0. I'll merge this for now. Thanks @asukaminato0721

@asukaminato0721 asukaminato0721 deleted the Default-values-of-arguments branch February 29, 2024 17:40
@adonis-jimenez
Copy link

Resolves #6721

Changes:

use default values of arguments, fix some missing ...args, use some native apis.

Screenshots of the change:

PR Checklist

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.

Default values of arguments
4 participants