Use standard directories on Unix and Windows #21
Conversation
|
Now that I think about it, the tests might need to be changed to account for this too. EDIT: Yep, errored. |
odensc
force-pushed
the
patch-1
branch
2 times, most recently
from
f847832
to
629e438
Compare
|
Fixed problems and rebased, tests should finish now. |
| @@ -27,6 +27,9 @@ function fail (err) { | |||
|
|
|||
| function openConfig (cb) { | |||
| var userHome = require('user-home'); | |||
| if (process.platform === 'linux' && userHome) { | |||
| userHome += '/.cache'; | |||
There was a problem hiding this comment.
I think you should scope this by if the platform is windows or not. OSX has a standard ~/.cache path as well. Also, if windows has a similar path, we should use it (and use path.join here as well).
|
Before we merge something like this I just want to check with @sindresorhus to see if he wants to support this directly in the |
|
That would probably be best fit for another module, isn't user-home only for getting the user home? It doesn't do anything else. |
|
Sure, but it could expose a second property? If @sindresorhus doesn't want
|
|
I already have a module for this: |
|
I feel like adding two modules just to userHome || os.tmpdir() and path.join(userHome, '.cache') is a bit overkill. |
|
Yeah, never mind the last suggestion, it's moot if you decide to use |
|
@TheSBros I did some digging about Windows, the standard cache directory would be |
odensc
changed the title
|
Alright, done. |
|
thanks @TheSBros! |
Use standard directories on Unix and Windows
|
published as |
|
Hmm, why was this reverted? I finally tracked down what was creating the annoying ".v8flags.foo.bar.json" in my home directory to this package, saw that the issue was already solved by this PR in |
|
Seems it has to do with this. Don't know why it was completely reverted rather than just making it create the folder. |
|
Yep, regressions abound were introduced. It will probably be re-added when #29 is fixed. |
|
@TheSBros it was completely reverted because I was in the middle of a consulting project when this change blew up. I'd love a PR that fixes the underlying problem so we can restore this functionality. |
|
Just went through the same hassle @tomxtobin, finally found the package doing this. ^^ So maybe #29 is fixed by #31 and then we could use standard directories! 🙌 |
|
I'd happily take a PR for this if you're willing to be the front line for the inevitable fallout. There are a fair few projects which depend on this: |
|
I think I would be willing... |
|
No sindre modules. Sorry. |
|
@phated are you serious or just joking? |
|
Serious, I have a bunch of examples why not to but the basic rule is "don't" |
|
Hmm that's a bummer, I used this module several times and helped with fixing a bug so implementing the logic another time here seems in vain. Since the whole module is only about 60 lines of code could you give it a chance/look? |
|
@phated do you happen to have them written down somewhere? |
|
Locking this. It's not the place to discuss this. |
|
@phated I think you're being a bit unreasonable here. If this functionality is something these folks want to add and they are willing to bear the responsibility of the impact, I see no reason to block them. @Siilwyn, please go ahead with your PR. If @phated is that hard-line about Sindre's modules, just inline the code and tests, the world will not explode because of the duplication. |
|
One of the reasons to not use his modules is that he is systematically dropping node 0.10 support from his modules (in some cases just to use an arrow function that makes no sense). All the gulp dependency tree is going to support node 0.10 for quite some time so we won't be depending on his modules. If you'd like to rewrite the code and give proper attribution to the original author or find a similar module, feel free to submit a PR. |
Not sure what the coding style is, so if I made a mistake please tell me.