Skip to content
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

Refactor temp dir logic #63

Merged
merged 1 commit into from
Jan 27, 2016
Merged

Refactor temp dir logic #63

merged 1 commit into from
Jan 27, 2016

Conversation

noamokman
Copy link
Contributor

Hi there!

Just replaced the temp dir logic with a module.
Better support for windows.

Thanks 😄

@noamokman
Copy link
Contributor Author

Ping 🔔
Is there a problem with the PR?

@Dexus
Copy link
Owner

Dexus commented Nov 17, 2015

Yes, i don't like to use more deps as needed. And the module is not needed.

Am 17.11.2015 um 05:40 schrieb Noam Okman notifications@github.com:

Ping
Is there a problem with the PR?


Reply to this email directly or view it on GitHub.

@noamokman
Copy link
Contributor Author

Hi @Dexus,
First of all, If you would look at the code you would see that this package covers windows cases that your line does not.
That is the main functional reason for this PR

The module is a polyfill for the os.tempDir function and it is helps you get a consistent behavior on different Node.js versions (even 0.8).

Also, in general, using small modules help make your code less complex, so you wouldn't have to know how to get the temp dir in your code.
you can simply get it from someone else who has done the job for you.
That is the beauty in Node.js and the npm ecosystem.

I highly suggest that you read this thread: sindresorhus/ama#10
It talks about single line modules and why it is important.

I just think it would help, and make the code a bit cleaner.
If you are still unconvinced,
The necessary changes to make this work for windows too is something like this:

var isWindows = process.platform === 'win32';
var trailingSlashRe = isWindows ? /[^:]\\$/ : /.\/$/;
var path = (os.tmpdir || os.tmpDir) && (os.tmpdir || os.tmpDir)();

if (isWindows) {
    path = path || 
        (process.env.SystemRoot || process.env.windir) + '\\temp';
} else {
    path =  path || 
        '/tmp';
}

if (trailingSlashRe.test(path)) {
    path = path.slice(0, -1);
}

Based on the original module.

I hope you reconsider. 😄

Dexus added a commit that referenced this pull request Jan 27, 2016
@Dexus Dexus merged commit a77bffa into Dexus:master Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants