-
Notifications
You must be signed in to change notification settings - Fork 5
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 XDG Specification compliance #8
base: master
Are you sure you want to change the base?
Conversation
if (process.env.XDG_CONFIG_HOME) { | ||
var rc_path = process.env.XDG_CONFIG_HOME+"/.bmndrrc"; | ||
} else { | ||
var rc_path = userhome+"/.bmndrrc"; |
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.
As far as I can tell, userhome
is now only being used here on L13. If so, its scope could be reduced.
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.
As in something like this?
if (process.env.XDG_CONFIG_HOME) {
var rc_path = process.env.XDG_CONFIG_HOME+"/.bmndrrc";
} else {
let userhome = process.env[(process.platform == 'win32') ? 'USERPROFILE' : 'HOME'];
var rc_path = userhome+"/.bmndrrc";
}
@@ -7,7 +7,12 @@ var fs = require('fs'); | |||
var argv = require('minimist')(process.argv.slice(2)); | |||
|
|||
var userhome = process.env[(process.platform == 'win32') ? 'USERPROFILE' : 'HOME']; | |||
var rc_path = userhome+"/.bmndrrc"; | |||
if (process.env.XDG_CONFIG_HOME) { | |||
var rc_path = process.env.XDG_CONFIG_HOME+"/.bmndrrc"; |
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.
DRY?
How about instead calculating the "rc_path_prefix" using the logic from this if else
and then appending the local folder to that, just once?
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.
Sounds like a good approach. How would you implement that?
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.
Upon second thought ...
While it makes sense to start the folder off with a period when it is directly in the home directory - so as to have it default hidden, doing so while under XDG_CONFIG_HOME
(which should point to $HOME/.config
unless configured otherwise) for the same reason makes less sense.
$HOME/.config
is the hidden folder and it subfolders need not be to avoid cluttering a listing of $HOME
.
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.
So change
var rc_path = process.env.XDG_CONFIG_HOME+"/.bmndrrc";
to
var rc_path = process.env.XDG_CONFIG_HOME+"/bmndrrc";
?
@@ -7,7 +7,12 @@ var fs = require('fs'); | |||
var argv = require('minimist')(process.argv.slice(2)); | |||
|
|||
var userhome = process.env[(process.platform == 'win32') ? 'USERPROFILE' : 'HOME']; | |||
var rc_path = userhome+"/.bmndrrc"; |
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.
I see these are all var
s and maybe that is standard javascript. 🤷🏿
At first glance a const
seems more appropriate.
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.
I think this was more about maintaining consistency with the rest of the file.
Add support for storing Beeminder credentials in XDG_CONFIG (as discussed here https://wiki.archlinux.org/title/XDG_Base_Directory) and defaults to ~/.bmndrrc if XDG_CONFIG not set.