refactored Option and Result to use a single class plus statics

[rrichardson]

From the department of "where were you two weeks ago?"

This changes does away with the distinct ResultType and Result functions, and merges everything into a class with statics.

I can understand why one might be reluctant to merge this, since the package just hit 1.0

This refactor is a breaking change, because it gets rid of the Result() and Option() functions. This is fine because, IMO, people should be using Result.from() and Option.from()

However...

That is literally the only change to the API.. I had to change 4 lines in the Unit tests to change the functions to use from(). All other tests pass.

Result(foo) and Option(foo) aren't really possibilities in the Rust stdlib anyways, so I don't know if people expect them in the API.

IMO it makes the implementation much more straightforward.

Oh.. I also changed the T symbol variable name to T_ just to keep myself from getting it confused with T the type parameter.

Also, big shoutout to @n_n from the Typescript Discord server who was a huge help and did much of the initial design.

[traverse1984]

A few things:

• As this is a breaking change, I can't merge it into 1.0, however small it is.
• I have considered this library structure throughout the project, and it also came up in this discussion on reddit. I am not sold that it's better.
• I'm not really happy with the idea of changing the symbol name for no other reason than you got mixed up. The T symbol is meant to be a private implementation detail - if it was part of the public API then I would be more understanding.

The current implementation makes it impossible to construct a defunct Option/Result at runtime unless you have really gone out of your way to do so. The use of a class at all is intended to be an implementation detail only, and should not matter to the user. I have also already had comments that some users wish there were no classes at all.

The biggest problem is that I can't see any real benefit of re-structuring in this way. What do you think this re-structure improves? I'd consider re-factoring for version 2 if I could see the benefit.

[rrichardson]

As this is a breaking change, I can't merge it into 1.0, however small it is.

We should absolutely follow semver practices. It does remove the ability to call Option() and Result(), so it is a major breaking change.

I'm not really happy with the idea of changing the symbol name for no other reason than you got mixed up. The T symbol is meant to be a private implementation detail - if it was part of the public API then I would be more understanding

I guess my take is that it doesn't matter what that variable name is defined to be. T is, to me, the worst possible choice. Since it it violates 2 general programming best practices.

1. It is a single letter variable scoped across an entire module. (Multiple modules, in fact).
2. It collides with a class-wide type parameter.

T_ isn't really any better. I should probably change it to HasValue or something similar.

The use of a class at all is intended to be an implementation detail only, and should not matter to the user. I have also already had comments that some users wish there were no classes at all.

I generally prefer to avoid classes myself, however, after many attempts, I've get to find a way to collect methods that is more succinct than a class. Especially in a scenario when you need the collection of methods to all be parameterized over the same types.

What do you think this re-structure improves?

Overall I think my intent of the refactor is this: Code is meant to be read more than it is written, and should be built to be maintained long after we're dead. The benefits of the refactoring are:

1. Better ergonomics with regards to error messages.
2. Principle of Least Surprise
3. Easier long-term maintainability.

I haven't benchmarked it, but I imagine it is the same speed as before, perhaps just a tiny bit faster because there is less indirection.

[traverse1984]

I will take on board the thoughts here. I think "Better ergonomics with regards to error messages" is a good point and the one most worth investigating. I'm still not convinced that moving to classes is an improvement.

Additionally, I don't think the pull request here is ready to be merged in its current state, because of the things I mentioned previously. Additionally, I really dislike the unreachable implementation. It feels like bloat and if we're talking 'principle of least surprise' - well, I found it very surprising and I had to determine where and how it was being used.

I'm going to close the pull request because even if all the issues are fixed, I still do not want to move over to class-based at this point. No-one else has been asking for this, but several others have said they wish it didn't use classes at all, so there's that too.