Instructor content

Dan Rathbun

A few more observations / issues:

File.exists? is deprecated.
Use the call File.exist? (singular) for future compatability.

The $LOAD_PATH members can be manipulated. Making an assumption about where the the first member points could cause problems. (Refering to the lins_get_default_instructor_folder() method.)

The lins_return_relative_path() method still references the deprecated PLATFORM constant, and assumes all PCs will running a MSVS compiled Ruby. The mingW compiled Ruby is becoming more popular and the Windows Ruby One-Click Installer uses only the mingW compiler now.
To test for Windows, Line 553 should be, either:
if( not RUBY_PLATFORM =~ /darwin/i )
or
if RUBY_PLATFORM =~ /(mswin|mingw)/i
The second has the advantage that, if in the future say a Sketchup Linux edition is released, you code will not break.

Instructions:
The inline instructions have grown to be very confusing for newbies.
I would suggest reverting to the simple default instructions, and add a note for advanced use see load_instructor_advanced.txt, and move all the advanced instructions into the separate text file.

And.. when this is all done (seeing as how you still have a beta version number,) and you decide on the first release, it would be best to create a new topic with a [ code ] prefix for version 1.0 (so Jim's indexer script can find it.) Then we will remove our beta version(s) this topic, and insert a link to the code release topic.

bentleykfrog

Dan, your advice is invaluable. There's still a couple of things I'm concerned about:

@dan rathbun said:

The way I had it.. it would use __FILE__ as a default for non-scrambled files, and insist on a explicit assignment of the @@tool_filepath variable for rbs files.
By instead, adding the required argument script_folder to the lins_setup() method, you have also defeated the default initialize() method that is mixed in (it has no arguments in the lins_setup() call.)
I think it will work better for newbie's the previous way.

The issue I had with the previous version was the way that load_instructor.rb constructed the default path to the /instructor/ folder. The previous version used __FILE__ from /mixin/load_instructor.rb and then navigated back to the folder that contained the /mixin/ folder. From that path it used /instructor/, so it meant that you would have issues if you had more than one ruby script using Load Instructor. So there needs to be a string attribute that identifies the name of the folder that contains /instructor/ to avoid this conflict between plugins. If the LoadInstructor mixin is going to be stored in a file that is separate from your script then calling __FILE__ will only be able to identify /load_instructor.rb and wont be able to identify the filename of the script that the LoadInstructor mixin was loaded into (unless there's a method for getting around this).

Regarding the @@tool_filepath issue, you should still be able to declare these paths in the second and third attributes of lins_setup()? like:

@@tool_filepath = VertexToolsPath

def initialize
  instructor_folder = File.join( @@tool_filepath ,'your_scripts_folder/instructor')
  temp_folder       = (RUBY_PLATFORM =~ /darwin/i) ? '' ; File.join(ENV["TEMP"], 'your_scripts_folder/instructor')
  lins_setup('',instructor_folder,temp_folder)
end

And if you're storing all the files beneath any of the load path's, you shouldn't need to worry about the second and third attributes, or @@tool_filepath for that matter, like:

def initialize
  lins_setup('your_scripts_folder')
end

@dan rathbun said:

The $LOAD_PATH members can be manipulated. Making an assumption about where the the first member points could cause problems. (Refering to the lins_get_default_instructor_folder() method.)

Hmmm, it should be best then if I threw an error if the /instructor/ folder can't be found, like (using your @@tool_filepath check):

def lins_get_default_instructor_folder(script_folder)
  instructor_folder = ''
  $LOAD_PATH.each {|load_path|
    check_path = File.join(load_path, script_folder, 'instructor')
    if check_path != '(eval)' && File.exist?(check_path)
      instructor_folder = File.join(load_path, script_folder, 'instructor')
      break
    end
  }
  if instructor_folder.empty?
    file_path = File.join(script_folder, 'instructor')
    raise(ScriptError,"#{self.class.name}; Instructor folder error. Could not find #{file_path} beneath any of the $LOAD_PATH's.")
  else
    return instructor_folder
  end #if
end #def

I've adopted all your other suggestions to the script, I just have to sort out the former issues before I'll produce the first release.

Dan Rathbun

Niall... I am sorry. I've got my head wrapped around another project, and I was not seeing the obvious here.

YOU cannot use __FILE__ at all, in your mixin code. Because it's a special Ruby keyword that gets evaluated when the mixin is parsed.

I was thinking wrongly that it's evaluation would be defered if it was used inside a method (the "setup" method.)

The tool author CAN use __FILE__ if his script is unscrambled, otherwise if he scrambles his tool code, he must use a rbs loader where he should set a constant (like ThomThom's example.)

The tool author only cares where YOUR code is when he uses the require() statement which should find it via the $LOAD_PATH array. Otherwise where the mixin file resides (it's path,) has no bearing on anything else. (Once it's loaded.. it is refered to as Mixin::LoadInstructor and it's path is irrelevant.)

So really yout mixin NEEDS to use the @@tool_filepath class variable in both cases (an rb or rbs.)

And the tool author needs to always set it to the path of his tool script file, either:
@@tool_filepath = __FILE__ # for unscrambled
or
@@tool_filepath = SOME_CONSTANT # (or a literal,) for scrambled rbs.
If the author is smart, they will wrap their loader and declare their path constant, in the same tool class namespace their rbs code in wrapped in.