untoldwind / KontrolSystem2

Autopilot scripting system for KSP2
Other
54 stars 14 forks source link

Request for comments: Result<T, E> overhaul #142

Closed untoldwind closed 4 weeks ago

untoldwind commented 3 months ago

With the new 0.5.7.0 pre-release (https://github.com/untoldwind/KontrolSystem2/releases/tag/v0.5.7.0), I would like to introduce a simplification of the Result<T, E> structure.

Copy&paste of the release notes:

Result<T, E> overhaul.

Background

Changes

lefouvert commented 3 months ago

Seen. Still thinking about what it implies. Some keywords are pleasant, as stack_trace.

I'll try to come back soon with constructive comments.

lefouvert commented 3 months ago

I interpret current version of Result as exceptions. Be able to customize it is really great. So I'm a little sad to see the second part of the Result disappear. In the other hand, even in professional context, I never had to create an exception with something more than a string message.

So, objectively, Result<T>, even if it's less flexible, their is no need to be as flexible as Result<T, E> and is a more concise way. And the tool added .stack_trace and .current_stack() are really a good idea.

I'm busy for this afternoon, but as soon as I come bback on KSP2, I install the prerelease in order to try thoses reworked Result<T>

untoldwind commented 3 months ago

The original Result<T, E> was designed after the corresponding variant in Rust (https://doc.rust-lang.org/core/result/enum.Result.html), where it makes sense to model the errors depending on the use-case (e.g. IOError vs. ParseError ...).

In the more scripting oriented world of TO2 this does not make all that much sense though. The most common use-case is: Show the error message and where the error occurred.

If there is a need to distinguish between different types of errors it would be pretty easy to add an additional .type (or .error_type field to the new core::error::Error.

As for Results = Exception ... That is something of a more philosophical question :) One might say: A result is something you expect to fail under certain conditions, while Exception are something you do not expect to fail (... but do anyway, because reasons)

lefouvert commented 3 months ago

The original Result<T, E> was designed after the corresponding variant in Rust (https://doc.rust-lang.org/core/result/enum.Result.html), where it makes sense to model the errors depending on the use-case (e.g. IOError vs. ParseError ...).

I don't know enough rust to recognize this, I've only seen it from far (even if I would like to invest more time in this language which smell good). Happy to learn this :)

In the more scripting oriented world of TO2 this does not make all that much sense though. The most common use-case is: Show the error message and where the error occurred.

I totally agree with this.

If there is a need to distinguish between different types of errors it would be pretty easy to add an additional .type (or .error_type field to the new core::error::Error.

I didn't feel the need. Maybe later, I will found a usage of this feature, but I'm not representative of all user of your mod, even if I'm a talkative one. Anyway, it seems to be a good idea.

As for Results = Exception ... That is something of a more philosophical question :) One might say: A result is something you expect to fail under certain conditions, while Exception are something you do not expect to fail (... but do anyway, because reasons)

I laughed. I only try to match new (for me) concepts with mind scheme I already have. It work for me, but it introduce a lot of wobble in my vocabulary and blur for the ones with more knowledge than me. My own philosophy is more about "don't catch anything. If it crash, it will be easier to identify their is something wrong" -insert «If he dies, he dies» meme-

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 60 days with no activity.

github-actions[bot] commented 4 weeks ago

This issue was closed because it has been inactive for 14 days since being marked as stale.