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

Range refactoring #2995

Merged
merged 4 commits into from
Jul 13, 2023
Merged

Range refactoring #2995

merged 4 commits into from
Jul 13, 2023

Conversation

dvd101x
Copy link
Sponsor Collaborator

@dvd101x dvd101x commented Jul 10, 2023

This is just an idea to refactor the function.

@josdejong
Copy link
Owner

Nice, I like it!

I think we can merge this PR as-is, or do you have more plans with it? Like merge the number and BigNumber functions together by passing an additional argument add 😉?

@josdejong
Copy link
Owner

(this argument add only needs the following implementations: (a, b) => a + b for numbers and (a, b) => a.plus(b) for BigNumber. Still, I'm not sure if that generalization is worth it)

@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Jul 12, 2023

I think there are two generalizatoins, add and isPositive. I validated it works by adding them as dependancies but I could also replicate the previous logic by making it's own functions add and isPositive. Would you recommend doing it that way instead of with the added dependancies due to performance?

@josdejong
Copy link
Owner

This looks very neat indeed! It makes a lot of sense to "just" use the standard functions add and isPositive. I like the simplicity of your solution a lot 😎

Yeah my only concern was a bit of performance optimization, but I think this is neglectable (and if really an issue, we can always optimize later).

Ok shall I merge your PR now?

@josdejong josdejong mentioned this pull request Jul 13, 2023
@dvd101x
Copy link
Sponsor Collaborator Author

dvd101x commented Jul 13, 2023

Thanks! Yes please

@josdejong josdejong merged commit 7923d58 into josdejong:develop Jul 13, 2023
7 checks passed
@dvd101x dvd101x deleted the range-with-units branch July 16, 2023 22:54
@josdejong
Copy link
Owner

The refactor has been published now in v11.9.0

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.

2 participants