-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support for user-specifiable name for cache directory #5
base: master
Are you sure you want to change the base?
Conversation
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.
Hey, thanks~!!
Left some comments for you.
Do you mind sharing your use case for this? IMO, it'd be better to have everything shared from .gittar
explicitly so that users don't end up with copies of the same tarball in different root locations.
Of course, this pkg isn't that popular to really make that a concern, but if that ever were the case, I'd be afraid of having something existing in multiple locations, when the underlying gittar
can know for certain if it has it. Also, current approach allows for easy cleanup -- single directory to remove.
lib/index.js
Outdated
function getTarFile(obj) { | ||
return path.join(DIR, obj.site, obj.repo, `${obj.type}.tar.gz`); | ||
function getTarFile(obj, name) { | ||
return path.join(name !== undefined ? path.join(HOME, '.' + name) : DIR, obj.site, obj.repo, `${obj.type}.tar.gz`); |
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.
This could be:
function getTarFile(obj, name) {
let dir = name ? path.join(HOME, `.${name}`) : DIR;
return path.join(dir, obj.site, obj.repo, `${obj.type}.tar.gz`);
}
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.
Easier to read IMO
lib/index.js
Outdated
dest = path.resolve(dest || '.'); | ||
return new Promise((res, rej) => { | ||
const ok = _ => res(dest); | ||
opts = Object.assign({ strip:1 }, opts, { file, cwd:dest }); | ||
opts = Object.assign({ strip:1 }, opts, { file, cwd:dest, name: undefined }); |
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.
This would have to be
opts = Object.assign({ strip:1, name:null }, opts, { file, cwd:dest });
Otherwise you're always overwriting a user-specified opts.name
with name:undefined
.
Used
null
here just for shorthand; that's not the problem in this case. :D
Thanks for the suggestions! I added this feature, because some users might worry about the |
Users should be able to choose the name of gittar's directory in
~/
.Instead of
.gittar
users can choose any name.Example
gittar.fetch
gittar.extract