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

Update math module to use new Num methods #198

Closed
avivbeeri opened this issue Apr 26, 2021 · 6 comments · Fixed by #204
Closed

Update math module to use new Num methods #198

avivbeeri opened this issue Apr 26, 2021 · 6 comments · Fixed by #204
Labels
good first issue Good for newcomers

Comments

@avivbeeri
Copy link
Collaborator

Wren 0.4.0 introduced more methods for the Num class, such as a built in min, max and clamp/mid implementation. Consult the Wren 0.4.0 release notes for a full list of added methods.

I think it would be good for DOME to defer to these implementations where available.

We should also add clamp as an alias for mid.

@avivbeeri avivbeeri added the good first issue Good for newcomers label Apr 26, 2021
@benstigsen
Copy link
Contributor

I think it would be good for DOME to defer to these implementations where available.

Agreed.

We should also add clamp as an alias for mid.

I do not think an alias is needed. I think that clamp is more descriptive where as I could imagine mid being confused with the average between two values.

@avivbeeri
Copy link
Collaborator Author

We already have mid (I named it based on the pico8 version of this method) and then Wren added clamp, which does exactly the same thing, so I don't see any harm in adding it as an alias, for people expecting the Wren-style to be present.

@benstigsen
Copy link
Contributor

Oh, I just thought that Wren added clamp for the first time, but if mid already exists then yeah okay, an alias would make a lot of sense.

@TheKing0x9
Copy link
Contributor

Should I take up and solve this issue?

@avivbeeri
Copy link
Collaborator Author

If you feel that you want to, I'm very happy for you to do so!

I keep issues like this open specifically for new people to contribute to the project :)

@TheKing0x9
Copy link
Contributor

Great! I'll send a PR right away

@avivbeeri avivbeeri linked a pull request Jul 13, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants