Cache directory respects XDG_CACHE_HOME (basedir spec) #214

jasonkarns · 2020-03-20T18:51:55Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
Instead of setting the cacheLocation to a directory in the user $HOME, this PR now respects the XDG Basedir Specification (specifically the XDG_CACHE_HOME environment variable).

Which issue (if any) does this pull request address?
fixes standard/standard#1501

Is there anything you'd like reviewers to focus on?
The XDG Basedir spec states that XDG_CACHE_HOME should be used (if set) to determine where to write cache files. It is recommended and conventional that tools create a self-named directory beneath cache-home for the files to live in. If the XDG_CACHE_HOME is empty or unset, a value of $HOME/.cache is used. (TMPDIR is used if HOME is unset/empty) Additionally, to mirror the existing support for storing multiple caches, a subdirectory for each major version is used beneath the linter's cache directory. (${XDG_CACHE_HOME:-$HOME/.cache}/standard/vX)

xdg-basedir is a package by sindresorhus which derives the appropriate values for the different XDG directories according to the spec (and respecting the fallback values). This package derives these values only once, when required. Because the values are cached, it is non-trivial to write tests to cover the various env-var scenarios. Due to this and the simplicity of the code change, I opted to not add any tests to the existing (simple) suite.

In place of tests, here are the various scenarios covered manually:

XDG_CACHE_HOME configured to a custom location:

$ XDG_CACHE_HOME=/my/cache node -p "new require('./').linter({cmd: 'linty', eslint: require('eslint')}).eslintConfig.cacheLocation"
/my/cache/linty/v0/

XDG_CACHE_HOME is unset or empty; (uses the XDG default: $HOME/.cache/)

$ XDG_CACHE_HOME= node -p "new require('./').linter({cmd: 'linty', eslint: require('eslint')}).eslintConfig.cacheLocation"
/Users/jasonkarns/.cache/linty/v0/

XDG_CACHE_HOME is unset or empty and HOME is unset or empty; (uses TMPDIR):

$ TMPDIR=/tempy HOME= XDG_CACHE_HOME= node -p "new require('./').linter({cmd: 'linty', eslint: require('eslint')}).eslintConfig.cacheLocation"
/tempy/linty/v0/

jasonkarns · 2020-03-20T18:56:00Z

I'm unsure what is going on with the tests. I get the same test failures locally when run from master.

TillaTheHun0 · 2020-03-25T03:16:57Z

@jasonkarns I am seeing similar on updates to #194 just now. The same tests fail for me locally, and also when I run them from the tip of master.

toddbluhm · 2020-04-30T09:09:12Z

While I do really like this I am a bit concerned by xdg-basedir packages first line in the readme file:

This package is meant for Linux. You should not use XDG on macOS and Windows. Instead, you should follow their platform conventions. You can use env-paths for that.

Maybe this should be using env-paths instead? I know xdg-basedir won't break because it has non-linux os safeguards in the code, but if we are going to make the change, maybe we should make it work for all platforms by using the appropriate cache location on each platform?

jasonkarns · 2020-04-30T12:46:48Z

@toddbluhm Personally, I don't agree with "You should not use XDG on macOS and Windows.". I'm on macOS and have been using XDG's locations for years.

I might agree for tools and apps that are built for non-developers. In that case, it makes sense to put data in ~/Library/** as per the apple reference guidelines. However, for developers who spend all day in Terminal, and likely have a dotfiles repo for versioning their configs, and use whole piles of *nix tools, I strongly strongly favor XDG's structure.

Windows, eh, I have no strong opinion. Though I would wager with the prevalence of git-bash, and WSL that even there, developer-focused tools should remain consistent with the XDG directories. (All the XDG defaults still live in $HOME, anyway, so XDG still works on Windows just fine.)

toddbluhm · 2020-05-02T06:57:27Z

@jasonkarns Thank you for that clarification. I do agree that since this is a developer tool it makes sense to keep the location in the respective HOME directories. I just wanted to make sure that the warning on xdg-basedir was brought up and considered before moving forward.

@mightyiam you still okay with this change since you responded on the initial ticket? @LinusU thoughts?

I am thinking this will probably require a major version change since it changes the cache location for all platforms.

mightyiam

👍

Major change.

mightyiam · 2020-05-02T07:04:24Z

index.js

@@ -7,7 +7,7 @@ var os = require('os')
 var path = require('path')
 var pkgConf = require('pkg-conf')

-var HOME_OR_TMP = os.homedir() || os.tmpdir()
+var CACHE_HOME = require('xdg-basedir').cache || os.tmpdir()


For reference:

https://github.com/sindresorhus/xdg-basedir/blob/a8b4130c61413006f1716d84b92a5ffd6e9336e8/index.js#L14
https://nodejs.org/api/os.html#os_os_tmpdir

Makes sense.

index.js

LinusU

LGTM

toddbluhm

Looks good. Just the one comment and a rebase and we can move forward on this.

Co-authored-by: Linus Unnebäck <[email protected]>

jasonkarns · 2020-05-04T14:27:49Z

Rebased!

mightyiam · 2020-05-04T14:33:18Z

index.js

@@ -38,8 +38,8 @@ function Linter (opts) {
  var m = opts.version && opts.version.match(/^(\d+)\./)
  var majorVersion = (m && m[1]) || '0'

-  // Example cache location: .standard-v12-cache/
-  var cacheLocation = path.join(HOME_OR_TMP, `.${this.cmd}-v${majorVersion}-cache/`)
+  // Example cache location: ~/.cache/standard/v12/


I suggest this comment be removed. It may be understood that that is the only possible pattern.

Suggested change

// Example cache location: ~/.cache/standard/v12/

mightyiam · 2020-05-04T14:34:22Z

When merging, squash merge into a single commit?

feross · 2020-10-27T22:02:30Z

Released as 13.0.0

TillaTheHun0 mentioned this pull request Mar 25, 2020

feat: add option to configure standard via rc config file #194

Open

mightyiam approved these changes May 2, 2020

View reviewed changes

LinusU reviewed May 2, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

LinusU approved these changes May 2, 2020

View reviewed changes

toddbluhm mentioned this pull request May 4, 2020

Publish 12.0.1 #221

Closed

toddbluhm approved these changes May 4, 2020

View reviewed changes

jasonkarns and others added 3 commits May 4, 2020 10:25

Adds xdg-basedir dep

97aa9b7

Respect XDG Basedir Spec for cache location

0c920fa

Update index.js

1a9c851

Co-authored-by: Linus Unnebäck <[email protected]>

jasonkarns force-pushed the xdg-cache branch from 2673f83 to 1a9c851 Compare May 4, 2020 14:26

mightyiam reviewed May 4, 2020

View reviewed changes

feross approved these changes Oct 27, 2020

View reviewed changes

feross merged commit afa026f into standard:master Oct 27, 2020

jasonkarns deleted the xdg-cache branch November 3, 2020 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache directory respects XDG_CACHE_HOME (basedir spec) #214

Cache directory respects XDG_CACHE_HOME (basedir spec) #214

jasonkarns commented Mar 20, 2020 •

edited

Loading

jasonkarns commented Mar 20, 2020

TillaTheHun0 commented Mar 25, 2020

toddbluhm commented Apr 30, 2020

jasonkarns commented Apr 30, 2020 •

edited

Loading

toddbluhm commented May 2, 2020

mightyiam left a comment

mightyiam May 2, 2020

LinusU left a comment

toddbluhm left a comment

jasonkarns commented May 4, 2020

mightyiam May 4, 2020

mightyiam commented May 4, 2020

feross commented Oct 27, 2020

Cache directory respects XDG_CACHE_HOME (basedir spec) #214

Cache directory respects XDG_CACHE_HOME (basedir spec) #214

Conversation

jasonkarns commented Mar 20, 2020 • edited Loading

jasonkarns commented Mar 20, 2020

TillaTheHun0 commented Mar 25, 2020

toddbluhm commented Apr 30, 2020

jasonkarns commented Apr 30, 2020 • edited Loading

toddbluhm commented May 2, 2020

mightyiam left a comment

Choose a reason for hiding this comment

mightyiam May 2, 2020

Choose a reason for hiding this comment

LinusU left a comment

Choose a reason for hiding this comment

toddbluhm left a comment

Choose a reason for hiding this comment

jasonkarns commented May 4, 2020

mightyiam May 4, 2020

Choose a reason for hiding this comment

mightyiam commented May 4, 2020

feross commented Oct 27, 2020

jasonkarns commented Mar 20, 2020 •

edited

Loading

jasonkarns commented Apr 30, 2020 •

edited

Loading