Code review comment for lp:~stocks29/do-plugins/scripts

Revision history for this message
Alex Launi (alexlauni) wrote :

You're still matching on file extensions, this is no good. What if you just indexed all of the files in a user specified scrips directory, ~/Scripts or something. The list of possible script types is too long to enumerate them all like that, you need a better solution. What about ruby, LOLcode, lua, boo, lisp, php, and the other 10000 scriptings languages that are out there, your plugin should not care what language the script is written in.

You still have the PerlScriptAction.cs file, You should remove this.

line 109 in ScriptAction you have a huge long error message, you should break that into a couple of lines, try not to exceed 120 chars per line.

the block starting at 118 can be replaced with a WaitForExit () call, http://msdn.microsoft.com/en-us/library/system.diagnostics.process.waitforexit.aspx

line 122 rather than p_result == string.Empty, use if (string.IsNullOrEmpty(p_result)) { }, I would do a ternary operation there, it'd be a bit more concise.

Rather than FileItem you should be using IFileItem, p_result should be pResult (but really should be something more descriptive. scriptOutput?).

Why do you depends on Do.File in your addin.xml? FileItem and IFileItem should be provided by core.

Looking good, just those couple of minor issues.

« Back to merge proposal