SketchUp / api-issue-tracker

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

Sketchup.require/load Docs #462

Open AntonSynytsia opened 4 years ago

AntonSynytsia commented 4 years ago

I just realized but Sketchup.require (and probably the alias Sketchup.load) does not throw errors if file does not exist or there is a loading error with the file. Instead, it only outputs the error to console and returns false. This is an unexpected behavior it differs from the normal Kernel.require , which actually throws an error in such cases.

I know that changing the API for these may break existing plugins, but it would be good to have this mentioned in the API docs.

DanRathbun commented 4 years ago

This may be related to a limitation on embedded Ruby subprocesses. Exceptions need to be trapped at some point especially during the application load cycle. This is why (during startup) all errors are queued up and spit out into a special LoadError listing dialog.

... it would be good to have this mentioned in the API docs.

Agree.

I know that changing the API for these may break existing plugins, ...

Then it's not likely to be changed. You can write your own loader method that uses Kernel#require when the file extension is .rb (during testing) and then uses Sketchup::require if the extension is .rbs or .rbe.

However ... the "Tools/extensions.rb" file would still use Sketchup::require in it's load() method, so the call wrapping wouldn't work during startup for loading an extension under test.


On a side note regarding Sketchup::load ... I believe there is an open request to make is work like Kernel#load in that it will reload encrypted files regardless of whether the file path appears in the $LOADED_FEATURES array.

~

AntonSynytsia commented 4 years ago

Thanks, Dan. I like your idea of having a custom require function.

thomthom commented 4 years ago

Yea, internally both Sketchup.require and Sketchup.load both call the same function - which uses a load guard.

I'm wondering if this is a case where we can/should accept a breaking change. Over the years of doing extension reviews I don't recall every seeing Sketchup.load being used. I suspect the impact would be minimal.

thomthom commented 4 years ago

I'm logging this as a bug - and make sure we have it re-reviewed. It would help the development process. (I don't think load in any form is used other than in debugging.)

DanRathbun commented 4 years ago

I'm wondering if this is a case where we can/should accept a breaking change.

I think we should. But it can also be a "softer breaking change".

For example ... Sketchup::load can be implemented to:

Ie, the first time a given filepath is loaded, Sketchup::load acts similar to Sketchup::require with respect to adding the filepath to the $LOADED_FEATURES array. But it never should bail out if the given filepath has already been loaded.

DanRathbun commented 4 years ago

So lets talk about whether Sketchup::load should raise exceptions (normally) or trap the exception and return the exception object as the result. (The normal "good" return value could be true to indicate good loading, ... or it could be false to indicate no exceptions occurred. I really do not know anyone who cares or needs to know whether it was the first load or a post-load require request like the Kernel#require returns.)

Would a use of Sketchup::load not trapping exceptions during the load cycle cause issues ?

sketchupbot commented 4 years ago

SU-45927

thomthom commented 4 years ago

Good point, Sketchup.require and Sketchup.load eating the loading exceptions has always bugged me. Changing Sketchup.require might be too risky, but given the rare use of Sketchup.load I think we could at alter it to not eat exceptions.

DanRathbun commented 4 years ago

I agree. IMO, it's used mostly in test loading.

AntonSynytsia commented 4 years ago

The only reason I am concerned with Sketchup.require/load eating exceptions is inability to use it for fallback loading - if an error occurs with one file, load the other, and so on... I believe fallback loading is useful in production mode just as in testing/development mode.

DanRathbun commented 4 years ago

In my experience, within a single extension consisting of multiple files, if an error occurs early in the file load, chances are it will affect other files yet to load. So it isn't this important to have the load loop continue.

However, if you are writing a load manager for multiple extensions, then yes you want a load loop that saves the exceptions, and will continue loading the next extension (or code repo, etc.)

This is easy to do with non-encrypted Ruby files and the global require method, but not so easy with Sketchup.require. Ie, when the loop finishes, the coder needs to be able to browse through a collection of load errors.

I've done this with a hash whose keys are the filenames, and values are the exceptions. Works great before encrypting, but fails afterward.

I've even tried to hook into the Exception constructor to grab references to all subclass instances as they are created, but failed (so far.) [My goal was to push the exception instances into a global $LOADERRORS hash. I've only tried on the Ruby-side. Perhaps this needs to be done on the C-side? I also have failed to get the global $! to return anything but nil.]

Some of us have tried to use #set_trace_func with no joy, as well.

DanRathbun commented 4 years ago

I've just noticed that #set_trace_func is deprecated in favor of a relatively new core class TracePoint. (ie, new for 2.x Ruby I think, so SketchUp 2014+)

I haven't yet tried it out.