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

Improve LinearFunction API #112

Closed
samreid opened this issue Oct 22, 2021 · 8 comments
Closed

Improve LinearFunction API #112

samreid opened this issue Oct 22, 2021 · 8 comments

Comments

@samreid
Copy link
Member

samreid commented Oct 22, 2021

During TypeScript porting with @jbphet, we saw that TypeScript was confused by our usage of LinearFunction. We say it is a @constructor and invoke it with new but we return a different function from the constructor. This causes type errors in TypeScript. We found things get better if we remove @constructor and new. We may also want to return a rich object with "evaluate" and "inverse" methods, or rename it to createLinearFunction.

@samreid
Copy link
Member Author

samreid commented Nov 2, 2021

I'm planning to turn LinearFunction into a class with an evaluate method. It will be instantiated with new. This will match up with other interfaces we use such as Transform1 and PiecewiseLinearFunction. There are around 70+ usages of LinearFunction, so they will need to all be updated.

@samreid samreid changed the title LinearFunction should not be invoked with new Improve LinearFunction API Nov 2, 2021
@samreid
Copy link
Member Author

samreid commented Nov 2, 2021

Everything is passing aqua locally, I think this is ready for commit.

samreid added a commit to phetsims/balloons-and-static-electricity that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/fourier-making-waves that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/wave-interference that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/energy-forms-and-changes that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/inverse-square-law-common that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/resistance-in-a-wire that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/energy-skate-park that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/quadrilateral that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/masses-and-springs that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/gravity-force-lab-basics that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/charges-and-fields that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/gravity-and-orbits that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/color-vision that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/capacitor-lab-basics that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/john-travoltage that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/scenery-phet that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/rutherford-scattering that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/faradays-law that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/ratio-and-proportion that referenced this issue Nov 2, 2021
@samreid
Copy link
Member Author

samreid commented Nov 2, 2021

I wasn't sure who to ask for review, so I ran

_.sample('JG, JB, JO, CK, SR, MK, CM'.split(' '))
'JG,'

@jessegreenberg can you please review? Close if all is well.

@samreid
Copy link
Member Author

samreid commented Nov 2, 2021

On second thought, I noticed the original author of LinearFunction was @pixelzoom, so perhaps @pixelzoom should review. The problem is described in #112 (comment) and the proposed solution (which was implemented) is described in #112 (comment).

@samreid samreid assigned pixelzoom and unassigned jessegreenberg Nov 2, 2021
samreid added a commit to phetsims/greenhouse-effect that referenced this issue Nov 2, 2021
samreid added a commit that referenced this issue Nov 2, 2021
samreid added a commit to phetsims/neuron that referenced this issue Nov 2, 2021
@pixelzoom
Copy link
Contributor

The proposed solution sounds great to me. I've always hated the original implementation - which was a consensus as I recall, to make call sites less verbose.

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Nov 2, 2021
@samreid
Copy link
Member Author

samreid commented Nov 2, 2021

Thanks, closing.

@samreid samreid closed this as completed Nov 2, 2021
@samreid samreid reopened this Nov 2, 2021
@samreid
Copy link
Member Author

samreid commented Nov 2, 2021

Upon reflection, it is unclear whether @pixelzoom reviewed the implementation or the proposal only. @pixelzoom can you clarify? Please close if all is well.

@samreid samreid assigned pixelzoom and unassigned samreid Nov 2, 2021
@pixelzoom
Copy link
Contributor

Implementation looks good, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants