SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
39 stars 10 forks source link

Named arguments for model.start_operation #623

Open thomthom opened 3 years ago

thomthom commented 3 years ago

Spin-off from #622

Instead of the current sequence of boolean arguments,

model.start_operation("Hello", true, false, true)

it would be nice to support named arguments:

model.start_operation("Hello", disable_ui: true, transparent: true)

This makes it easier to omit the other optional arguments.

Note that "next_transparent" is deliberatly omitted from the example above as it's a deprecated argument we want to steer people away from. It's only there as a last resort to workaround a couple of other operation bugs.

DanRathbun commented 3 years ago

I agree.

It is also can be called on older versions (back to SU2014) as the named arguments are ignored. (Ie, the method just takes on the default argument settings and will not cause an ArgumentError exception.)


I would also suggest changing the result doc. I think it would always return true and otherwise raise either ArgumentError or TypeError. (Ie, I do not think false would ever be a result for this method.)

[In hindsight, perhaps it should have just returned the model object to allow call chaining.]

thomthom commented 3 years ago

We could also throw in block mode while we're at it:

model.start_operation("Hello", disable_ui: true, transparent: true) do
  # Do stuff here.
end

Though, how to deal with aborting or commiting an operation upon errors... 🤔

model.start_operation("Hello", disable_ui: true, abort_on_error: true) do
  # Do stuff here.
end

Not sure if abort_on_error is best defaulting to true or false.

or... I might be overthinking this, it might still be doable to allow this:

model.start_operation("Hello", disable_ui: true, transparent: true) do
  # do stuff
rescue
  model.abort_operation
end
DanRathbun commented 3 years ago

We could also throw in block mode while we're at it:

Yes I have long been an advocate of the block form for operations where the commit or abort is automatically (and appropriately) called.

This means usually that the begin ... rescue is internal to the start_operation method and wraps the yield call. It is the whole point of making them easier for both newbs and the experienced.

I'm not sure if there is a way to detect within the method body if the passed block has an associated rescue clause ? (Ie ... if rescue_given? or similar.) Meaning a way to defer in inner rescue and let the outer rescue clause do the error handling and abort.

Not sure if abort_on_error is best defaulting to true or false.

Returning false on an abort for a block form might be the only way to let the outer code do some error handling. Like:

successful = model.start_operation("Hello", disable_ui: true, transparent: true) do
  # the block to yield
end
if not successful
  # the op was aborted, handle error
end

This could be nested to remove the successful reference ...

if not model.start_operation("Hello", disable_ui: true, transparent: true) do
    # the block to yield
  end
  # the op was aborted, handle error
end

Another option is to raise a custom exception ... Sketchup::OperationError or whatever, that the code could trap with an outer rescue clause.


Block form operation discussion is mostly in issue https://github.com/SketchUp/api-issue-tracker/issues/52 and a undoable block operation in issue https://github.com/SketchUp/api-issue-tracker/issues/4.