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

[10.x] Add a new Numberable helper #49036

Closed
wants to merge 12 commits into from

Conversation

caendesilva
Copy link
Contributor

@caendesilva caendesilva commented Nov 17, 2023

Abstract

Building on #48845, this class will work similarly to the Stringable class, but for numbers.

This was requested on the Discord and by several people on Twitter, and mentioned in #48849

Development

Feedback and contributions are much appreciated! I can be reached here, or in the #internals Discord.

  • Add accessors for the Number class
  • Add methods to interact with the value
  • Add Number::of() and number() shorthands
  • See if we can find a better name for this helper? (Numerable has been suggested)

Building on laravel#48845, this class will work similarly to the `Stringable` class, but for numbers.
I'm making so that these helpers modify the underlying value, instead of creating new instances like in the Stringable class. This is because I think modifying the values is more helpful for the kinds of use cases I see for this feature.
Based on laravel@d41e885, instead of the newer laravel@a75dea9 as there are no facade methods where the latter functionality adds value to this case.
@rmunate
Copy link
Contributor

rmunate commented Nov 18, 2023

#48845 (comment)

@maartenpaauw
Copy link
Contributor

Would be a great addition, but wouldn't it be better to make this class immutable like the stringable class?

@caendesilva
Copy link
Contributor Author

Would be a great addition, but wouldn't it be better to make this class immutable like the stringable class?

Thanks for the feedback! It definitely could be, but I went this route because I personally think modifying the values is more helpful for the kinds of use cases I see for this feature being used. But I am 100% open to changing it to be immutable if that turns out to be the consensus on what's best.

@bert-w
Copy link
Contributor

bert-w commented Nov 19, 2023

This makes every equation like ($x + 5)/3 unreadable when you write it in functional fluent mode Numberable($x)->add(5)->divide(3).

I would never use this, unless there's actually logic inside that would be worth abstractifying like vector or matrix math.

@caendesilva
Copy link
Contributor Author

caendesilva commented Nov 19, 2023

This makes every equation like ($x + 5)/3 unreadable when you write it in functional fluent mode Numberable($x)->add(5)->divide(3).

I would never use this, unless there's actually logic inside that would be worth abstractifying like vector or matrix math.

That's a great point, and a strong reason to use immutable versions. I'll make a patch tomorrow. Thanks for the feedback!

@Rizky92
Copy link

Rizky92 commented Nov 19, 2023

I'm not sure about this PR. Stringable works because there is a need for complex string manipulation fluently.

Your previous PR is great for converting unit of measurement or making numeric values readable, but not for manipulating it.

@bert-w's comments is a strong concern. Unless you provide another abstraction for working with arithmetics, I don't really see a good thing why this PR is necessary.

@caendesilva
Copy link
Contributor Author

I'm not sure about this PR. Stringable works because there is a need for complex string manipulation fluently.

Your previous PR is great for converting unit of measurement or making numeric values readable, but not for manipulating it.

@bert-w's comments is a strong concern. Unless you provide another abstraction for working with arithmetics, I don't really see a good thing why this PR is necessary.

Good points as well! Thanks for all the feedback! Going to try to refine this tomorrow, then open up for review to see what the team thinks. I appreciate your thoughts!

@gocanto
Copy link
Contributor

gocanto commented Nov 20, 2023

🤝 Sincere and humble question,

Did you evaluate the possibility to use Money/Money? - This lib is battle tested and offer this functionality out of the box.

cc @taylorotwell @driesvints

@caendesilva
Copy link
Contributor Author

🤝 Sincere and humble question,

Did you evaluate the possibility to use Money/Money? - This lib is battle tested and offer this functionality out of the box.

cc @taylorotwell @driesvints

I haven't considered it for this feature, as I think these helpers are best used for quick formatting. If one needs something more robust, I think that's when you would reach for a library like yours. In my opinion, specialised code like that does not need to be in the Laravel core as we have packages to get the functionality when needed.

Consensus reached in the pull request: laravel#49036

It also removes the tappability as that loses it's functionality when immutable.
@caendesilva caendesilva marked this pull request as ready for review November 20, 2023 11:52
@taylorotwell
Copy link
Member

I don't really think this is necessary yet. It's common to chain many string operations together when working with strings - I don't think the same is really the case with numbers.

@caendesilva
Copy link
Contributor Author

I don't really think this is necessary yet. It's common to chain many string operations together when working with strings - I don't think the same is really the case with numbers.

Fair enough! It was requested by some people after my last PR, and it was fairly quick to make this one so I thought I'd throw it out there in case anyone finds it useful. Thanks for taking the time to review it!

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.

8 participants