Implement op_once for form builder

[dodomorandi]

@lu-zero let me know if you'd rather tweak the struct names, i.e. a better one for FormBuilderInner.

[codecov]

Codecov Report

Merging #105 (9ac8bf8) into master (18e0a14) will decrease coverage by 0.19%.
The diff coverage is 87.60%.

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
- Coverage   96.95%   96.77%   -0.19%     
==========================================
  Files          11       11              
  Lines       12670    12884     +214     
==========================================
+ Hits        12284    12468     +184     
- Misses        386      416      +30     

Impacted Files	Coverage Δ
src/builder.rs	97.31% <87.17%> (-0.57%)	⬇️
src/builder/affordance.rs	96.55% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[lu-zero]

I changed it to use 3 states:

• ANY -> op and op_once
• MANY -> op only
• NONE -> no way to add more ops

Potentially we could do away the outer FromBuilder and have a from() that collapses ANY/MANY to NONE once we save it back in the ThingBuilder.

[dodomorandi]

I looked at your changes, and after thinking a bit I still have the impression that it is more convoluted that it should be. This corresponds to your implementation:

flowchart LR
    thing((Thing)) -->|form| any[CAN_ADD_ANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|op_once| none[CAN_ADD_NO_OPS]
    many[CAN_ADD_MANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS]  -->|from| Unlocked[FormBuilder::Unlocked]
    many[CAN_ADD_MANY_OPS] -->|from| Unlocked[FormBuilder::Unlocked]
    none[CAN_ADD_NO_OPS] -->|from| Locked[FormBuilder::Locked]
    Unlocked[FormBuilder::Unlocked] --> Form
    Locked[FormBuilder::Locked] --> Form

Loading

The only practical difference between this implementation and the previous one is that you cannot call op_once after op. But this does not make any sense from a developer perspective:

• No one is gonna explicitly use op_once, because it's overly restrictive -- if I need one operation, I can just use op once and I am fine.
• If you have a layer that prepend an operation and you only need that one to be used, you are just creating the builder with op_once, therefore it is not possible to add another operation anyway.
• If you have a layer that append an operation and at the same time you want to constrain the number of operations, you are doing it wrong. It is totally unexpected that something that a hidden layer triggers a compilation error because "you should not have done something". In this case the hidden layer should work before the user, not after.

Given that this approach is just to fix some compile-time constraints elsewhere and that these constraints can be easily broken just deserializing a JSON representation (because you are not enforcing anything at runtime), I still think that this approach is more complex than it should be.

[lu-zero]

As you may see in wot-serve branch I needed to bind the http{get,post,patch} to the ANY state (no ops present) and we can do away the FormBuilder enum by having From implemented for a final state (either NONE or a FINAL one).

flowchart LR
    thing((Thing)) -->|form| start[CAN_HREF]
    start[CAN_HREF] -->|href| any[CAN_ADD_ANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|op_once| none[CAN_ADD_NO_OPS]
    many[CAN_ADD_MANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS]  -->|from| final[FINAL]
    many[CAN_ADD_MANY_OPS] -->|from| final[FINAL]
    none[CAN_ADD_NO_OPS] -->|from| final[FINAL]

Loading

For the wot-serve use-case I might even consider adding a CAN_ADD_ONCE state that let you call op only once.

[dodomorandi]

Sorry, but I still don't get it. In your branch the only constraint you are creating is that the returned form does not have any operation set. This could be just implemented without op_once, tracking the fact that an operation has been added using a const bool. Now it feel even more convoluted than before...

[lu-zero]

Indeed we can potentially not have an op_once at all since we could use the state to have

flowchart LR
    thing((Thing)) -->|form| start[CAN_HREF]
    start[CAN_HREF] -->|href| any[CAN_ADD_ANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|from| once[CAN_ADD_ONE_OPS]
    once[CAN_ADD_ONE_OPS] -->|op| final[CAN_ADD_NO_OPS]
    many[CAN_ADD_MANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS]  -->|from| final[CAN_ADD_NO_OPS]
    many[CAN_ADD_MANY_OPS] -->|from| final[CAN_ADD_NO_OPS]
    once[CAN_ADD_ONE_OPS] -->|from| final[CAN_ADD_NO_OPS]

Loading

And have From instead to restrict from ANY to ONCE and only have fn op implemented for each intermediate state.

[dodomorandi]

....which, again, brings to the fact that your last state is just the final FormBuilder, therefore your 3-state collapses into a boolean. Right?

[lu-zero]

Using more than a boolean let us avoid the FormBuilderInner, I started poking at it for that reason and then I realized it can be leverage to express more while I tried to use it in wot-serve.

With the last commit I can correctly restrict op when using http_{get,put,...} so you can set it only once.

[dodomorandi]

Sorry, I unable to follow you anymore. From your previous message and the relative flowchart it was pretty clear it was possible to remove one state, you just added one instead. Feel free to go along with the idea you have in mind, it is pretty difficult to me at this point to write an implementation that makes sense in your head but not in mine.

[lu-zero]

Here what I meant.

I managed to reduce the states compared to the diagram above, I'm afraid we cannot collapse everything further in a single binary state, but we do not have FormBuilderInner anymore :)

flowchart LR
    thing((Thing)) -->|form| start[CAN_ADD_ANY_OPS+NO_HREF]
    start[CAN_ADD_ANY_OPS+NO_HREF] -->|href| any[CAN_ADD_ANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS] -->|from| once[CAN_ADD_ONE_OPS]
    once[CAN_ADD_ONE_OPS] -->|op| final[CAN_ADD_NO_OPS]
    many[CAN_ADD_MANY_OPS] -->|op| many[CAN_ADD_MANY_OPS]
    any[CAN_ADD_ANY_OPS]  -->|from| final[CAN_ADD_NO_OPS]
    many[CAN_ADD_MANY_OPS] -->|from| final[CAN_ADD_NO_OPS]
    once[CAN_ADD_ONE_OPS] -->|from| final[CAN_ADD_NO_OPS]

Loading

(message delayed by a series of meeting)

[lu-zero]

We ended up adding the constraints directly in wot-serve.

In WoT 2.0 we might have either this constraint applied or not necessary anymore depending on what we'll have specified for the relationship between op and protocol binding.