Fixing issues #2 and #10, and adding custom name feature. #11

Merged
merged 8 commits into from Apr 2, 2013

Projects

None yet

4 participants

@rafaelrinaldi
Contributor

Pack a single .js file with custom output

You can now compile and compresses all .js files into a single one by using --compile/-c. Example:

Compiling two .js files into a new file called all.js

yuglify foo.js bar.js -c all.js

Compiling two .js files into a new file with the default name which is main.min.js

yuglify foo.js bar.js -c

Template based custom name

Now you're able to customize the output files by using --name/-n. It's basically a sprintf-based template where you have the file base name and its extension. The default value is %(name)s.min%(ext)s:

yuglify foo.js bar.js

This command will generate two files following the default file name template, foo.min.js and bar.min.js. Let's say you want to use a dash instead of a dot:

yuglify foo.js bar.js -n '%(name)s-min.%(ext)s'

This command will generate two files: foo-min.js and bar-min.js.

I've tried to follow the code standards you've been using. Let me know if you want me to make any change.

Cheers.

@foxxtrot foxxtrot and 1 other commented on an outdated diff Feb 23, 2013
bin/yuglify
@@ -48,6 +59,8 @@ if (parsed.help || (!parsed.argv.remain.length && !parsed.terminal)) {
' --type js|css what type of file to compress (used with stdin)',
' --terminal use stdin and stdout instead of files',
' --output <file> use this file when using --terminal',
+ ' --name/-n <template> custom sprintf-based file name template',
+ ' --compile/-c <file> compiles and compresses all files into a single one.',
@foxxtrot
foxxtrot Feb 23, 2013

Renaming this argument 'combine' would make more sense to me. Uglify is compiling either way.

@rafaelrinaldi
rafaelrinaldi Feb 23, 2013 Contributor

I agree! Will change this on Monday.

@foxxtrot foxxtrot commented on the diff Feb 23, 2013
package.json
@@ -1,35 +1,37 @@
@foxxtrot
foxxtrot Feb 23, 2013

Why did you change all the formatting of this file? Makes it unclear what is new.

@rafaelrinaldi
rafaelrinaldi Feb 23, 2013 Contributor

I did not changed this file. Maybe a GIT issue?

@foxxtrot
foxxtrot Feb 24, 2013

Well, git would only have changed the file in it's repo if you'd marked it for committing, and the file has been completely reformatted (it appears those are the only changes) If you truly didn't intend to modify this file, please revert the change with:

git checkout 2ca3f117ae -- package.json

@foxxtrot foxxtrot and 1 other commented on an outdated diff Feb 23, 2013
bin/yuglify
@@ -48,6 +59,8 @@ if (parsed.help || (!parsed.argv.remain.length && !parsed.terminal)) {
' --type js|css what type of file to compress (used with stdin)',
' --terminal use stdin and stdout instead of files',
' --output <file> use this file when using --terminal',
+ ' --name/-n <template> custom sprintf-based file name template',
@foxxtrot
foxxtrot Feb 23, 2013

A template format feels really overkill to me, why not just re-use the 'output' argument?

@rafaelrinaldi
rafaelrinaldi Feb 23, 2013 Contributor

Well, I see this as an extra and convenient feature. I needed to change the way things were named once and thought about implementing this right on the compression tool would be great instead of having a separate task to handle this.

@foxxtrot
foxxtrot Feb 24, 2013

My inclination is to say that the two features are different enough to be handled in separate pull requests, but that's largely because I find value in the combining function that I don't find in the template function. I think the renaming a file based on some template is better handled by another pass in the build process, rather than inside of YUglify.

I also object to 'yet another' templating setup used here, and would rather see YUglify use something consistently used elsewhere in YUI or the YUI build chain (another reason to support not doing this in YUglify, as you could configure your own build toolchain to use whatever templating solution you wanted if the file copies and renames where handled elsewhere in your toolchain).

The implementation is fine, I just don't think this belongs here.

@JohnArcher
Contributor

Would be great to have these features, as I really need to change the standard min file name.

@JohnArcher
Contributor

Any idea, when or whether this pull request will be merged, @davglass ?

@davglass
Member

I'm not completely convinced this is needed, I've been letting it simmer for a bit to see if anyone else wants this in.

I can potentially see the -c option, but the templating to me is over engineering. Most build systems have rename commands, so adding this into a build system and having it rename the output file to me is the proper thing to do.

If this was split into two pull requests, one for each feature. The -c would likely have already been merged and the template part closed.

@rafaelrinaldi
Contributor

Yeah, I added the template thing just for convenience because I found
myself renaming the output files very often. I was using my own build
system and thought it would be nice if I could handle the renaming via the
compression tool, since it is responsible for generating the actual output
files.

It's been a while since I posted a comment here, I was very busy in a
project over the past month. I will put some love on it today and I will
keep the custom template feature just for convenience on my own fork. In
case you change your mind... :)

Cheers guys.

On Wed, Mar 27, 2013 at 9:32 AM, Dav Glass notifications@github.com wrote:

I'm not completely convinced this is needed, I've been letting it simmer
for a bit to see if anyone else wants this in.

I can potentially see the -c option, but the templating to me is over
engineering. Most build systems have rename commands, so adding this into a
build system and having it rename the output file to me is the proper thing
to do.

If this was split into two pull requests, one for each feature. The -cwould likely have already been merged and the template part closed.


Reply to this email directly or view it on GitHubhttps://github.com/yui/yuglify/pull/11#issuecomment-15520514
.

Rafael Rinaldi
rafaelrinaldi.com

@JohnArcher
Contributor

Thanks for your responses, guys. Although I also wouldn't mind to have the templating in it :-), it is ok, if it is not. You are right, it should be easy to rename it externally.

Would be cool, though, to have the -c option in it, so I am looking forward to @rafaelrinaldi split. :-)

@rafaelrinaldi
Contributor

Alright guys, so here is what I did... I've removed the -n/--name feature per @davglass's request also removed the vendor implementation of sprintf. Now I'm using native features of node.
If you simply pass -c after a batch of file paths, it will combine the files using the default base name which is "main", though you can set a custom base name by passing a string to the command, like -c app.

Here are some examples for your better understanding:

yuglify foo.js folder/bar.js -c

Will generate the file main.min.css on the project folder.

yuglify foo.js folder/bar.js foo.css folder/bar.css -c

Will generate the files main.min.js and main.min.css on the project folder.

yuglify foo.js folder/bar.js -c app

Will generate the files app.min.js on the project folder.

yuglify foo.js folder/bar.js foo.css folder/bar.css -c app

Will generate the files app.min.js on and app.min.css on the project folder.

Want to hear your opinions on this.

@JohnArcher
Contributor

👍 Yep, looks very good to me! Want! :-)

@JohnArcher
Contributor

Hm, I thinking of adding an option like overwrite/append for the output file(s). Background: I want to add a comment at the beginning of the output file, so I would create the file first (with the comment) and have yuglify append its generated content to that file. What do you think?

@rafaelrinaldi
Contributor

@JohnArcher I don't know if I get it right but I think that maybe it's a better idea to have this feature within your build script or something?
I'm trying to focus this pull request only on the --combine feature, as @davglass suggested in a previous post.

@JohnArcher
Contributor

Yep, no problem, @rafaelrinaldi I also think that the build script is the better place for that.

@davglass
Member
davglass commented Apr 2, 2013

@rafaelrinaldi Thanks for cleaning this up for me, I'll merge this in today.

@davglass davglass merged commit a34f6a1 into yui:master Apr 2, 2013

1 check passed

default The Travis build passed
Details
@davglass
Member
davglass commented Apr 2, 2013

Published in yuglify@0.1.3

@JohnArcher
Contributor

👍 Thanks!

@JohnArcher
Contributor

Hm, I tested it with an Ant script in Jenkins. It seems it have problems with the escape quotes for the color output. I get lines like this: Successfully generated JS file: �[32mjs/debug/jquery.timepicker.addon.min.js�[0m Anyone ones whether it is possible to tell Jenkins to interpret that? Or: do we need that color stuff?

@davglass
Member
davglass commented Apr 2, 2013

Jenkins doesn't support colored term output unless you use a plugin (I think).

Let me look into this, there is a way to tell of the terminal can accept colors and if we should print them.

@davglass
Member
davglass commented Apr 2, 2013

Install the latest from master and see if that works for you, if it does I'll push a new version.

@JohnArcher
Contributor

@davglass Yep, works like a charm. You just forgot to add that code also for single file compression case (see my pull request). Thanks for the help!

@davglass
Member
davglass commented Apr 2, 2013

@JohnArcher Merged yours in, published as yuglify@0.1.4.

@rafaelrinaldi
Contributor

That was a good discussion. Cheers guys!

@JohnArcher
Contributor

@davglass Thanks!
@rafaelrinaldi Indeed, that was nice and productive. My first that kind experience on GitHub. Thanks, guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment