[lldb-dev] command script import

[Jim Ingham]

Sorry, a few more quibbles.  

Why not add FileSpec::GetFilenameStrippingExtension or something like that as well, since that's what you really use?  That seems like another useful function.  Don't take out the extension one, that also seems good.

What a pain that you can't import a Python module w/o adding its containing directory to the Python Path!  There must be some reason why, but sigh...

Why do you have to do this bit:

+    FILE *tmp_fh = (python_interpreter->m_dbg_stdout ? python_interpreter->m_dbg_stdout : stdout);
+    
+    {
+        Locker py_lock(python_interpreter, tmp_fh);

That seems non-obvious and at least merits a comment.

Finally since there seem to be several reasons why LoadScriptingModule could fail in this function, it would be better if it took an Error & parameter and filled that out appropriately.

Jim


On Oct 14, 2011, at 12:55 PM, Enrico Granata wrote:

> Hi all,
> hopefully the third time is the charm :)
> 
>> The only quibble I have is that you do a bunch of path manipulation by hand here that should be done through FileSpec.  
> 
>> Also, instead of pulling off the .pyc or .py extension by hand it would be good to add this functionality to FileSpec, since that is also generally useful.
> 
> I have modified the implementation following Jim's advice, and now I use FileSpec directly. I have also added a method named GetFilenameExtension() to said class.
> 
>> As to the test case, no, it does not belong in the python_api subdirectory.  You can create a
>> functionalities/command_script subdir to house the import dir.
> 
> It looks like you had already created functionalities/command_script. I have created an import subdirectory there and moved my test case files there.
> 
> Thanks for the feedback on this patch.
> 
> Sincerely,
> <importcmd.diff>
> - Enrico Granata