-
Notifications
You must be signed in to change notification settings - Fork 2k
[CS2] Fix #2870: Allow specifying output filename #4661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…s a file and not a path, save as the desired filename
src/command.coffee
Outdated
| opts.output = path.resolve opts.output if opts.output | ||
| if opts.output | ||
| outputBasename = path.basename opts.output | ||
| if '.' in outputBasename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about --output dir/. and --output dir/sub/..? I guess those two are supposed to mean --output dir/ and --output dir/? Also, there's --output . and --output ...
Also, a question: Is it possible to add a trailing slash to force a directory? --output dir/script.js/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was afraid of testing this, since this part of the codebase has no tests.
All those cases are ones I didn’t consider. Let me cover them . . .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so now if path.basename (which is everything from the last separator, / or \, to the end of the string) returns only periods, I’m just treating the output argument as a path (folder). This conforms to the old behavior, though it doesn’t seem to make much sense.
If the output argument ends in a slash, I’m forcing it to be treated as a folder name, even if it contains periods. So coffee -co scripts.js/ test.coffee will save into scripts.js/test.js.
… even if that folder name would contain a period (e.g. /scripts.js/); if output filename is only periods, treat it as a path
|
I wonder if we should keep this unmerged until 2.0.1, or if we’ve merged in so many bugfixes that we should release a 2.0.0-beta5. I’m eager to release 2.0.0, but I don’t want it to have too much code that hasn’t been more widely tested. |
src/command.coffee
Outdated
| outputBasename = path.basename opts.output | ||
| if '.' in outputBasename and | ||
| not helpers.ends(opts.output, path.sep) and | ||
| not /^\.+$/.test(outputBasename) # Not `.` or `..`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three or more dots is a valid file name, though. We should only special case . and ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You gotta be kidding me.
|
I vote for a 2.0.0-beta5 btw. |
|
Yeah I’m thinking that too. Might as well merge this in then 😄 |
Fixes #2870. If the input is a file, not a folder; and
--outputis a path to a file, likedist/file.jsorbuild.js; save as the desired output path/filename.Examples: