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

POC: Turn Currency into a class #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gliljas
Copy link

@gliljas gliljas commented Apr 17, 2021

Inspired by the comment in issue #83, which makes a lot of sense. Currency is not similar to a value type.

Also turned Money in a readonly struct, and custom JSON serialization support was needed as a result.

@michaeldileo
Copy link

Commenting on this because I happened to see it while submitting an issue. I have a lot of code that depends on currency not being nullable and this would break a lot of that. This is definitely a breaking change for a lot of people.

@gliljas
Copy link
Author

gliljas commented May 26, 2021

Yes, it would definitely warrant a new major version, but I still would strongly recommend that this change is made. No Money value would ever have a null currency, and C# 8's nullability could help.

@michaeldileo
Copy link

I personally am not wild about throwing an argument exception in the Money constructor. Replacing with a "null object" pattern would probably be better. That would help prevent from breaking existing code.

However, my personal preference is to not make this fundamental change, but I'm also not an actual contributor on this lib.

@gliljas
Copy link
Author

gliljas commented May 26, 2021

I personally am not wild about throwing an argument exception in the Money constructor. Replacing with a "null object" pattern would probably be better. That would help prevent from breaking existing code.

That could indeed be an option, but why would we throw an exception, unless it's explicitly called with a null Currency instance?

However, my personal preference is to not make this fundamental change, but I'm also not an actual contributor on this lib.

Neither am I. It's just that NodaMoney is the closest thing to a .NET Money standard there is, and this design flaw is a bit......perplexing. It would be nice to see NodaMoney reach the maturity that NodaTime has, although of course, there's no actual link except for the Noda name.

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