Builder Pattern applied to the Builder struct, errors instead of panics

[ConnyOnny]

implements #7

[WaDelma]

First of all thanks for the pr!

Overall changes look good, but I have problem with the errors that result from wrong usage of the builder API itself and I would like to hear your rationale on them.

Atleast the name of the configuration struct has to be renamed before merge. Or if you can convince me that it's good idea, then all other places I have used same reasoning needs to be changed to keep the API consitent :P.

[WaDelma]

Having configuration object way better than what was before. I felt weird having all kinds of methods taking builder as parameter.
I would change the name to just Configuration as I have done with Generator, Algorithm and others.
The idea is that from outside perspective we have poisson::Configuration instead poisson::PoissonConfiguration.
If the user has conflict with their own type/trait or with another librarys they can always do:
use poisson::Configuration as PoissonConfiguration;.

[WaDelma]

Hmm... When I was first writing the code I didn't go with custom error as all the errors were caused by missuse of the API and thus programming errors which means that there isn't anything meaningful to do if code triggers them.
Maybe if one calculates radius via other code before setting it via builder then it could have use?
So maybe custom error is better anyways.

[WaDelma]

I also don't really like having *Unspecified errors as they are coding errors and there is nothing to do about them run time.
And *MultiplyDefined errors seem odd too as setting value twice isn't nessacary error and can just restrict the APIs usage.
If the errors were encode type system ala session types I would feel better about them. But that would feel bit too heavy for this.

Have to think about this.

[ConnyOnny]

Thanks for the reply.

The MultiplyDefined errors are a matter of taste. I can remove them if you want it that way.

The Unspecified errors would be spotted early in development of a simple application, so using a panic instead seems not too bad at first. But if the setup code for the Builder becomes more complex - say the radius is set in branches of a match statement and the erroneous case (where the radius is not set) is not well-tested - this could crash the whole application, which could be something large - like a game server or web server - with expensive outages. I try to do everything I can in order to avoid crashes. In my world, a Rust program is supposed to never crash, even if some parts are not properly tested. Okay, if the user just calls unwrap on the Result, there's nothing we can do, but IMHO we should do everything we can to not panic. Also the Error enum is in place anyway, so doing this doesn't really hurt.

I like session types. I don't like that implementing them involves so much manual labor. Tracking the Builder API changes all over the code base already was quite a pain. However if we do session types, then defining the same option multiple times should really not be permitted by the type system, in my opinion.

About the naming of the configuration struct you are absolutely right! I'll change it, probably tomorrow.

[ConnyOnny]

What is your opinion on the with_poisson_type function: Should it be required or should the Builder just use Type::Normal if it is not given?

[WaDelma]

I get what you mean by not wanting to crash ever, but if your code comes up into case where no radius is specified what can you do to recover from it? I do agree now that panicing was bad choice anyways: It doesn't show up in the type signature.

I should really play around with the session type idea as that would fix the problem. Maybe macros could be used to reduce the boiler plate involved? Would have to use generics for the builder because macros cannot create indents.

Maybe this error type solution should merged meanwhile until viability of session types is confirmed...

I think that using Type::Normal would be fine as default. If it's used default for builder Default trait should propably be implemented for it.

[WaDelma]

Here is the start of implementing the session type based builder pattern with macro. Setters aren't implemented yet as they need more complex token tree munching. Also to implement error checking there needs to be a way to provide custom build method.

[WaDelma]

and here is version that has working setters.

It doesn't yet work with generic or optional variables and doesn't include custom build method.

[ConnyOnny]

Wow, that's some impressive macro magic!

but if your code comes up into case where no radius is specified what can you do to recover from it?

For example there could be a popup "error generating tree positions" and the game could run without trees.

[WaDelma]

and now it has optional variables.

[WaDelma]

Thanks! macro_rules is powerful, but pain to work with. Good thing that there is resources like this to help with them. (I mostly learned how to use it by coding this monster of a macro)

Ah.. Interesting. I didn't think anything like that. That is basically same idea that led to how html/php/javascript or any web technology handles errors. Which is one reason I don't really like them and are attracted by static typing (Keeping broken program running is asking for trouble imho). But I do understand that you don't really want to crash your server. But in other hand you propably are catching panics coming from job threads in them anyways.

I can now see where you are coming from and having those errors would be good. Or just using session type builder :P

[WaDelma]

I think I have sunken into rabbit hole... Now it has support for single constraints on generic values.
Note: Playpen times out sometimes when trying to build that. Not sure why as it works perfectly fast locally.

[ConnyOnny]

That is basically same idea that led to how html/php/javascript or any web technology handles errors.

It's a huge difference whether the language tries hard not to crash or you try not to crash. If at some point an array is filled with NaNs instead of trees, because the language thought it was smart to fail as late as possible, you're f...rankly not in a very nice situation. I do not defend the concept fail-late or its employment by any language such as JavaScript, PHP, Ruby, Python etc.
But if you know that an error occured and know what the error is, the situation is very different, because you might be able to do something reasonable (which a programming language cannot).
To stay in the tree generation example, it could also say "map loading failed" and the user could try to play on another map, where the problem maybe does not occur.

I think I have sunken into rabbit hole

Nice. See what you can find down there and maybe put it in a crate and bring it back up, so everyone can profit from your journey. I just recently thought about the "session types" concept: "Why is there no crate for this?"

[WaDelma]

Now it has full generic support.
I had to restructure it slightly and so it now generates this kind of API:

pub mod Builder {
    use super::*;
    pub struct O;
    pub struct I;
    pub struct Builder<T1, T2, ..., Tm, C1, C2, ..., Cn>
       where //bound that affects Cs
    {...}
    pub fn new<T1, T2, ..., Tm, C1, C2, ..., Cn>() -> Builder<O, O, ..., O, C1, C2, ..., Cn>
       where //bound that affects Cs
    {...}
    // Setters. These require that there is O in the place that is set
    // and return Builder where that place is replaced with I
    impl Builder<I, I, I, ...> {
        pub fn build(self) -> Config {...}
    }
    pub struct Config<C1, C2, ..., Cn>
       where //bound that affects Cs
    {...}
    impl Config<C1, C2, ..., Cn>
       where //bound that affects Cs
    {
        // Getters
    }
}
pub use Builder::new as Builder;
pub use Builder::Config;

which is kinda funky:

• Builder::new works, but it's not static method on struct Builder, but static function in module Builder
• I pub use the new function as function Builder
  • I added this before realising that Builder::new worked and I kinda like it, but it could be confusing.
• I have to have unique name on the module and I didn't want to you to need to give it separate name as parameter.
  • Because you cannot create identifiers from thin air I had to use either Builder or Config
• Using pub use on Config is needed to make refering it's type manageable (struct member, etc.).
• Making O and I public is nessary to be able to have new as function.
  • They are used to encode the presence or absence of value.

[WaDelma]

Ah... I see your point clearly now. Lets remove all possible panics that user can cause and replace them with session types and error types then.

[WaDelma]

As custom derives become stable with macros 1.1, I went and implemented builder generation library with them. It should work on most use cases (even more than then the macro solution), but it's pretty ugly code.

[ConnyOnny]

Using bob is currently blocked WaDelma/bob#1
Otherwise bob looks great! (except that it could really use some comments 😄)

[WaDelma]

The issue should be resolved and I added some documentation.
I would love if you could give feedback on the documentation.

[WaDelma]

I updated the library to use alga (which means it works with newer nalgebra versions). This causes some conflicts with this pr.

Should I update the pr or are you willing/wanting to do it yourself? Also the issue that you reported for bob which prevented using it should be resolved. Have you had time to look at it? Would like some feedback on it before putting it to crates.io

[ConnyOnny]

Sorry, I didn't have time to look into it. bob is magic for me, as I don't know much about Rust's mighty macro system. I'm afraid in the near future I won't have the time to look deeply into bob and to update this PR.