-
Notifications
You must be signed in to change notification settings - Fork 687
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
Replace all concatenated paths with Node's path.join #307
Conversation
function getConfig() { | ||
return require(path.resolve(this.HOME) + "/config"); | ||
return require(path.resolve(this.CONFIG_PATH)); |
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.
Remove path.resolve
here.
c8df0d6
to
d946286
Compare
@xPaw, I believe I have addressed all your comments. However, this PR needs a lot of testing. I have done some, but definitely happy to have others test too. If done badly, this can create and use config in wrong directories, so people would believe their config was wiped. |
if (!fs.existsSync(config)) { | ||
mkdirp.sync(Helper.HOME, {mode: "0700"}); | ||
fs.writeFileSync( | ||
config, | ||
fs.readFileSync(__dirname + "/../../defaults/config.js") | ||
fs.readFileSync(path.resolve(path.join( | ||
__dirname, |
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 uses something else, I'm thinking the goal is the same. Should we stick with one or the other?
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.
__dirname
is correct.
Okay, I've fixed |
a96bfbb
to
f955aa1
Compare
f955aa1
to
ad65915
Compare
ad65915
to
d6f2def
Compare
c1977d1
to
ca06e63
Compare
Alright, I'm all set with this PR! (Removing @thelounge/maintainers, could you review and test this please? :-) |
Review looks fine, but I'll set it up so I can test it tomorrow probably (quite busy tonight). |
@maxpoulin64, do you mind giving this a review as well? |
…erywhere Replace all concatenated paths with Node's path.join
My attempt at fixing #301.
Let me know if this solution is crappy or should be improved, I don't claim this is the one solution and I'm happy to get feedback. Also, I hope I didn't forget anything.