pyjscompressor multi-platform

[pjbraby]

Replaced all specific OS commands with the general python lib versions
Works for me on windows now, should work elsewhere assuming these added python functions work there:
os.remove()
shutil.rmtree()

[anthonyrisinger]

COMPILER is preferred to match env var, not much reason to deviate. consider setting via arg (using optparse, or just sys.argv), and falling back on the environment variable

[anthonyrisinger]

for the most part your changes look fine to me -- nice work -- thanks :-)

just address the handful of comments i've made and we'll go ahead and pull.

[pjbraby]

Ok, so i think ive made a bit of a mess of this whole fork-commit-etc thing.
Anyway:

• I guess it's better to use command line arguments so people dont have to edit the script(so new versions will work as expected) right? In light of this i just removed the option to edit it into the script.
• I see what you mean about the COMPILER variable name, I somehow didn't really make the connection between the environment variable and my variable.
• the directory size was only ever used on the script's target directory, this functionality is replaced by just counting the sizes of all files before and after compression as they are compressed and outputting this value at the end
• as far as I can tell both scripts only used spaces and most of my indentation was generated by IDLE. Searching for \t in jedit had no results in either version, maybe you just mean the indentation style?
• sorry about the whitespace, I tend to add a lot of space around things when i'm working and I guess i got over excited when cleaning it up, I can see this might be a bad habit if it will be going through this diff checker thing

Some general things:
You said about not removing whitespace and I suppose this applies generally to style changes, when you said to commit first do you mean that I should do a second commit with just style changes to my first one or just leave them out entirely?

I understand I shouldn't just rearrange someones code how I like it, it's because this file seemed distinctly inactive and that I made so many changes that I thought it would be ok.

Anyway I hope it's ok now, any more tips always appreciated

[anthonyrisinger]

@PJSHAB, this looks pretty good -- there is only one thing I'd like you to change before i merge this: subprocess.call(). this is good (i wanted you to use it), but you've still got the command as a single string and are using 2> /dev/null, which should be avoided. the call() function is really just a shortcut for this class:

http://docs.python.org/library/subprocess.html#popen-constructor

... take a look at the options there:

• pass the command as a list of arguments. then things work correctly if any args have spaces in their paths.
• use os.devnull instead of 2> /dev/null as this will work for Windows and Linux

what i'm looking for in the end is something like this:

retcode = subprocess.call(args=['java', '-jar',
                                COMPILER, ...,
                                '--js_output_file', js_output_file],
                          stderr=open(os.devnull, 'w'))

as per your whitespace questions -- don't worry about it too much for this merge -- but i meant you should put the functional changes in this commit, then whitespace/style changes in another commit. as a rule of thumb, they should always be separate, not only for ease of reviewing, but also because it's much simpler to revert changes if they are not mixed (eg. if the commit is deemed bad and breaks stuff ... whitespace changes can cause unnecessary conflicts and make it harder to revert).

[pjbraby]

Ok, another version:
Had to rearrange the exceptions a bit because it behaved differently with the list of args than with the string.
Also a couple of fixes: hitting an empty file no longer breaks it (div by 0 in get_compression) and the temp files get deleted now when the compiler produces an error and causes an exit.

Other thoughts:
In some small testing I found simple_optimisations to work on a recent version of opera, do we know what was causing the problem there and might it now be fixed?

[anthonyrisinger]

awesome! this is looking really great. final two tweaks :-) ...

• change stderr=open(os.devnull, 'w') to stderr=subprocess.STDOUT (i didn't realize you wanted to redirect both)
• add yourself to the top of this file, the CREDITS file, and the copyright file (it's a bit of a mess right now)

... after that i'll pull straight away!

[anthonyrisinger]

meh i'll pull this now, thanks! just be sure to add yourself top credits if your not there already.