Split activate_run_target into two methods

[danth]

Forced activation will be implemented first, so splitting it makes it possible to just add the queue method later, rather than having certain combinations of arguments which cause an error.

I think this will also make the usage more readable, as the behaviour is explained in the method name, rather than a boolean argument where you have to read the documentation to know what it does.

[danth]

Maybe it would be better to just remove queue_run_target for now, and add that code when we come to implement it, rather than having it commented out.

It is mostly the same as force_run_target so it would be easy to recreate later.

@NicolasFussberger What do you think?

[NicolasFussberger]

Maybe it would be better to just remove queue_run_target for now, and add that code when we come to implement it, rather than having it commented out.

It is mostly the same as force_run_target so it would be easy to recreate later.

@NicolasFussberger What do you think?

I think its a good idea.
Regarding the naming I would tend to keep the "activate", so something like activate_run_target_force?

What do you think about this approach in general @SimonKozik?
I agree to daniel that it allows a smoother transition path until the queuing is implemented.
We could even deprecate the method at some point and move back to the bool param if that is preferred once it is supported.

[SimonKozik]

Maybe it would be better to just remove queue_run_target for now, and add that code when we come to implement it, rather than having it commented out.
It is mostly the same as force_run_target so it would be easy to recreate later.
@NicolasFussberger What do you think?

I think its a good idea. Regarding the naming I would tend to keep the "activate", so something like activate_run_target_force?

What do you think about this approach in general @SimonKozik? I agree to daniel that it allows a smoother transition path until the queuing is implemented. We could even deprecate the method at some point and move back to the bool param if that is preferred once it is supported.

I'm not a big fan of just changing API, especially when it took some time to agree on.

Which problem are we trying to solve here?

1. We do implementation in stages, so we need solution to let user know this.
2. API is not very readable as the param is modifying the way the API is behaving.

If we are trying to solve the first problem, then we should simply return kRequestQueueIsFull (if force == false) as we currently have queue size of one.

If we are trying to solve the second problem, then we should understand that the queue is always there. I'm not sure what are we going to gain, by having a separate method to empty the queue. But if we want to go this way, then maybe following names could be the starting point for discusion:

enqueue_run_target_activation(RunTargetName runTargetName);  
force_run_target_activation(RunTargetName runTargetName);

If we go this way, then I can see comments saying that force_run_target_activation() is not very intuitive because you need to read documentation to find out... That we are not just jumping in front of the queue, but we are cancelling all previous requests and then we are adding this request at the front of the queue.

@danth and @NicolasFussberger which problem are we trying to solve?

[danth]

which problem are we trying to solve?

Problem 1, to get #489 merged.

I thought we could also solve problem 2 at the same time. But that is not the main goal.

If we are trying to solve the first problem, then we should simply return kRequestQueueIsFull (if force == false) as we currently have queue size of one.

I think it would be difficult for a new user to understand why all their calls are failing with this error.

We would have to add a new error code for kNotImplemented. That is easy to do, but it would mean we have an unused error code left over once the implementation is done.

you need to read documentation to find out... That we are not just jumping in front of the queue, but we are cancelling all previous requests and then we are adding this request at the front of the queue.

The same problem exists in the current design, where you simply see a true or false and have to check the documentation to know what it changes.

I suppose then that splitting into two methods is not much better than the current API, and we should just go with returning an error code to avoid making any unnecessary changes for now.

[SimonKozik]

We would have to add a new error code for kNotImplemented. That is easy to do, but it would mean we have an unused error code left over once the implementation is done.

No we only need to document (in know limitations) that currently the size of the queue is 1.
However please remember that we need to do this during the release process and I would assume, that we will implement the queue before we hit v0.10.

We should remember that merging to main is not the same as releasing the product... And we should also remember that head of the main is not the supported product / release... People are not usually using the latest version from git, unless they know what they are doing and they need to do this.

[SimonKozik]

@danth reading your replay and mine again... I would say that the client side of the library should limit the number of requests to 1. This should be easily hacked with a static variable and should be good enough as temporary solution.

Maybe a simple check on the force flag is not necessary the way to go.

[NicolasFussberger]

@danth reading your replay and mine again... I would say that the client side of the library should limit the number of requests to 1. This should be easily hacked with a static variable and should be good enough as temporary solution.

Maybe a simple check on the force flag is not necessary the way to go.

Then I would be in favour of rather returning kNotImplemented error code when force=true.
Its not user friendly (you would expect the default arguments to work), but at least its clear to a user this has not been implemented yet.
I am not sure if the queuing behaviour would be part of the next release.

[danth]

Sounds like kNotImplemented will be the easiest and most agreeable solution.

I'll close this, and add the error code as part of #489 since it is only a few lines to change.