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

Add Number.rounding_mode + .{with,save}_rounding_mode #11097

Closed
wants to merge 1 commit into from

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Aug 15, 2021

@Sija Sija force-pushed the number-with-rounding-mode branch 2 times, most recently from db65c04 to 70b7fc7 Compare August 16, 2021 21:20
@Sija Sija force-pushed the number-with-rounding-mode branch from 70b7fc7 to 728d70a Compare August 16, 2021 21:23
@Sija Sija changed the title Add Number.{with,save}_rounding_mode Add Number.rounding_mode + .{with,save}_rounding_mode Aug 16, 2021
@straight-shoota
Copy link
Member

I would prefer to start a discussion about this feature proposal first, instead of jumping direclty into a PR.

@Sija
Copy link
Contributor Author

Sija commented Aug 23, 2021

@straight-shoota I already referenced the comments that were voiced before.

@straight-shoota
Copy link
Member

Yes and in #7126 (review) I explicitly asked for a separate discussion. This is not a trivial addition. We should make sure to identify the correct use cases and design an approriate API for that.
Nothing like that has happend yet.

@Sija
Copy link
Contributor Author

Sija commented Aug 23, 2021

This is not a trivial addition. We should make sure to identify the correct use cases and design an approriate API for that.

This sounds like over-complicating the issue.

The use case is the ability to globally change the default rounding mode since it's common that an external code:

  • uses its own Number::RoundingMode as a default, which requires re-implementing this exact behavior in every shard that might want have such functionality.
  • uses Number#round without allowing the developer to pass the mode argument to it. The only other option is to monkey-patch each and every method that does that, which is pretty awful.

API in this PR covers that, adding a class property + two yielding class methods.

@Sija
Copy link
Contributor Author

Sija commented Sep 8, 2022

Could this be moved forward?

@straight-shoota
Copy link
Member

I'm not convinced about this implementation. So no approval from me without a dedicated discussion about use cases and alternatives.

@Sija
Copy link
Contributor Author

Sija commented Sep 8, 2022

@straight-shoota It's hard to discuss "not being convinced" statement. Could you please elaborate on what exactly makes you not convinced? btw, re: use cases I've already started a discussion that you didn't continue.

@beta-ziliani
Copy link
Member

Hi @Sija , we took some time to discuss this with the team and concluded we will not move this forward. While we see that changing the behavior of the default rounding method is a reasonable request, we also believe that doing it on the fly, globally, provides an entry point to bugs. Moreover, the block-yielding methods makes it sound as if they could contain the global effect, but that's not really the case.

Number.with_rounding_mode :ties_away do
  ties_away?
  ... # no ties away 😭
end

def ties_away?
  Number.rounding_mode = :ties_even # 😈
  ...
end

Therefore, I will close this PR, as I see no point in keeping it around. We can always re-open it if we change our minds. As mentioned, we encourage you to continue the discussion in a dedicated issue where we can weight other alternatives.

@Sija
Copy link
Contributor Author

Sija commented Sep 10, 2022

Hi @beta-ziliani, thanks for looking into it.

While we see that changing the behavior of the default rounding method is a reasonable request, we also believe that doing it on the fly, globally, provides an entry point to bugs.

In order to minimize the scope of possible bugs stemming from globally changing the default, it could be changed to be a read-only property, changeable only within the scope of with_rounding_mode method.

Moreover, the block-yielding methods makes it sound as if they could contain the global effect, but that's not really the case.

Well, it is the case, but in your example you're changing the global default within the ties_away? method, effectively overwriting the applied default within with_rounding_mode. Usually you'd use one or the other method exclusively.

@beta-ziliani
Copy link
Member

In order to minimize the scope of possible bugs stemming from globally changing the default, it could be changed to be a read-only property, changeable only within the scope of with_rounding_mode method.

Note that this would easily break in a concurrent setting.

Well, it is the case, but in your example you're changing the global default within the ties_away? method, effectively overwriting the applied default within with_rounding_mode. Usually you'd use one or the other method exclusively.

Well, the thing is that you might not know how the methods you call will behave w.r.t. the rounding option, so you might work under the assumption of one option, when in reality it changes mid-way the computation.

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

Successfully merging this pull request may close these issues.

4 participants