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

Revert "Implicit operator removed" #17

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

Conversation

mcintyre321
Copy link
Owner

Reverts #16

@mcintyre321
Copy link
Owner Author

@Paul-Williams I've just accepted, then reverted your PR!

I have just reviewed #10, which adds


        public static bool operator ==(ValueOf<TValue, TThis> a, object b)
        {
            return b is TThis other && a == other;
        }

        public static bool operator !=(ValueOf<TValue, TThis> a, object b)
        {
            return !(a == b);
        }
        

With those operators in place, and the implict cast method, we get the following results

void Main()
{
	Foo foo = Foo.From("asd");
	Foo2 foo2 = Foo2.From("asd");
	("asd" == foo).Dump(); //true
	("asd" == foo2).Dump(); //true
	(foo2 == foo).Dump(); //false 
}
class Foo : ValueOf<string, Foo> { }

class Foo2 : ValueOf<string, Foo2> { }

Does this solve your original issue with the implicit cast, or am I missing something?

@Paul-Williams
Copy link
Contributor

Paul-Williams commented May 6, 2021

This code does not seem to work as intended.
It breaks your equality test:
Assert.IsTrue(clientRef1 == "ASDF12345");

Additionally the test:
("asd" == foo).Dump(); //true
breaks, if inverted to:
(foo == "asd").Dump(); //false

It also still allows the comparison of two different ValueOf types, which although is not a bug, is a design choice I think goes against the idea of avoiding primitive obsession. It means you could still compare an EmailAddress to a UserName. This, I think is what this class is intended to avoid.

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