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

[Refactor] vtkOpenGLTexture create* methods to use named arguments #3222

Open
floryst opened this issue Feb 25, 2025 · 2 comments · May be fixed by #3224
Open

[Refactor] vtkOpenGLTexture create* methods to use named arguments #3222

floryst opened this issue Feb 25, 2025 · 2 comments · May be fixed by #3224
Labels
good first issue 👋 type: feature request 💡 Desired capabilities / enhancements

Comments

@floryst
Copy link
Collaborator

floryst commented Feb 25, 2025

Motivation

Some of the vtkOpenGLTexture.create* methods are starting to accrue many parameters (see #3221 and #3077 changes for parameter additions). Example methods:

I would like to propose changing these methods to use named parameters over positional parameters. (See this comment for more context.)

This issue is for tracking as well as soliciting opinions over implementation approaches and whether this is something to move forward on.

Tagging @finetjul @jourdain @sedghi

Detailed Description

All named parameters

One approach is to move completely to named parameters:

publicAPI.create3DFilterableFromDataArray = ({ width, height, depth, dataArray, preferSizeOverAccuracy = false } = {}) => { ... }

This will name every parameter, but also means that leaving out a required parameter might not be as obvious (which can also be an issue with very long positional params, but I don't consider that a fruitful point of discussion for this suggestion).

My vote is for this option, but there is another option to consider.

Optional named parameters

We can instead make only the optional parameters part of an "options" record:

publicAPI.create3DFilterableFromDataArray = (width, height, depth, dataArray, { preferSizeOverAccuracy = false } = {}) => { ... }

This makes sense for optional params, but we still have the issue with many positionals (e.g. is it width/height or height/width in create3DFilterableFromDataArray(100, 100, ...)?)

Backwards compatibility

  • These are mostly used internally, so cutting a new major release is a soft requirement.
  • If we moved these from arrow functions to regular functions, we could detect when arguments.length > 1 and unpack the arguments accordingly.
@floryst floryst added good first issue 👋 type: feature request 💡 Desired capabilities / enhancements labels Feb 25, 2025
@sedghi
Copy link
Contributor

sedghi commented Feb 25, 2025

Yes please!

@finetjul
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue 👋 type: feature request 💡 Desired capabilities / enhancements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants