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

Fix os.tmpdir bug #240

Merged
merged 1 commit into from
Jan 12, 2016
Merged

Fix os.tmpdir bug #240

merged 1 commit into from
Jan 12, 2016

Conversation

BYK
Copy link
Contributor

@BYK BYK commented Oct 6, 2015

The checked method on os module was tempDir which was not existing on any version of Node (should have been tmpDir). With Node 0.10, the name became tmpdir.

The checked method on `os` module was `tempDir` which was not existing on any version of Node (should have been `tmpDir`). With Node 0.10, the name became `tmpdir`.
@nfischer
Copy link
Member

I agree, it looks like it should say tmpDir, not tempDir. With v5.3.0 I can see both tmpDir and tmpdir. Is there a difference in this case?

Could you provide an example of buggy behavior and the expected behavior?

@BYK
Copy link
Contributor Author

BYK commented Dec 30, 2015

@nfischer - you can check the ticket on ESLint that refers to this one: eslint/eslint#4061

In short, instead of using the built-in tmpDir functionality, it falls back to the manual temporary directory creation which is prone to errors.

@nfischer
Copy link
Member

After looking into things a bit, I agree with the change of os.tempDir -> os.tmpDir. However, I think that os.tmpDir is exactly equivalent to os.tmpdir (see the current source code, the archived source, and the stack overflow post that lead me there).

Unless there's discussion of getting rid of os.tmpDir in favor of os.tmpdir, we should probably only keep os.tmpDir (if this name is deprecated though, I think it's better to include both).

Other than that, this looks like something we would merge fairly soon.

@nfischer nfischer self-assigned this Jan 10, 2016
@nfischer nfischer added fix Bug/defect, or a fix for such a problem medium priority labels Jan 10, 2016
@BYK
Copy link
Contributor Author

BYK commented Jan 10, 2016

@nfischer I also double checked from my console in Node 4 and os.tmpdir === os.tmpDir returns true. That said the docs make no mention of this: https://nodejs.org/api/os.html#os_os_tmpdir

So my suggestion is either dropping the undocumented tmpDir version and also drop Node 0.8 support which is quite old, or keeping it as it is, for the sake of clarity. (we may add a comment linking back to this discussion).

@nfischer
Copy link
Member

Ok, you raise a good point about it being undocumented. Since this operation should only be performed once (and the result will be cached), it shouldn't bloat things too much. A comment linking back might be a good (plus, it'll trigger the CI so we can verify that it doesn't break things).

LGTM, once the CI passes

@nfischer
Copy link
Member

@ariporad what are your thoughts?

@ariporad
Copy link
Contributor

@nfischer: I'm not sure I understand this 100%, but I think that the best thing to do would be (os.tempDir || os.tmpDir || os.tmpdir)(...).

@nfischer
Copy link
Member

@ariporad os.tempDir doesn't actually exist (it was a typo I think).

You suggestion works if at least one of the three functions exists. The issue is that early versions (before 0.8) don't have any of those functions. So if os.tempDir and os.tmpDir don't exist, it'll always call os.tmpdir() (which doesn't always exist).

If we do it in multiple lines as it is, it will never call the function unless it actually exists (so the code works as expected).

As this PR is written, it fixes the typo (which is good), and it prefers the newer (and actually documented) name os.tmpdir over os.tmpDir, which is probably a safer bet in the long run.

@ariporad
Copy link
Contributor

@nfischer: Then LGTM!

nfischer added a commit that referenced this pull request Jan 12, 2016
@nfischer nfischer merged commit 5d1ccec into shelljs:master Jan 12, 2016
@BYK BYK deleted the patch-1 branch January 12, 2016 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug/defect, or a fix for such a problem medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants