-
Notifications
You must be signed in to change notification settings - Fork 933
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
Admit optional CLI arg specifying config file path #1254
base: master
Are you sure you want to change the base?
Conversation
This is useful when bootstraping a new workstation.
This removes some minor assertion duplication, and should be easier to test.
Also, try to use slighty more explicit arg, function names.
And stub the rest for now.
@lra friendly bump 😄 |
Yep, it's a big changes, and review take almost as much time as writing them, but it's in the pipe unless it's closed ;) |
@lra I know this it's been awhile, but I'm setting up a new machine and running into this issue again 😀It seems to be on others' minds, too, e.g. #1559. I'm happy to clean up the conflicts and make any other suggested changes, if you think you'll have time to review this? Given the state of the world right now though, I'd certainly understand if you can't get to this for awhile longer. |
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 would simplify the _resolve_config_path()-method. In my opinion, it would be reasonable to expect users to provide a path/to/.configfile.cfg. e.g.
$ mackup restore --config=~/.my-config-file.cfg
# or
$ mackup restore --config=~/my/dotfiles-repo
My suggestion:
def _resolve_config_path(cls, filename):
if filename is None:
# use the default configfile
config_file = os.path.join(os.environ["Home"], MACKUP_CONFIG_FILE)
else:
# resolve relative paths and expand commonly used "~"
config_file = os.path.abspath(os.expanduser(filename))
if not config_file.endswith(".cfg"):
# user input was only path/to so add the default config_filename
config_file = os.path.join(config_file, MACKUP_CONFIG_FILE)
# check if file actually exists
if not os.path.exists(config_path):
error....
return config_file
Nb: conflicts fixed in: mackup/config.py mackup/main.py tests/config_tests.py
@tillhainbach yeah, tilde expansion seems like a good idea. I've added that, and fixed the merge conflicts. While making those changes, Python 2 issues kept popping up. That got me wondering: what's the thinking around whether to continue maintaining backwards compatibility with an officially deprecated version of the language? (I don't mean to get this PR off course--the issue merits a separate thread--but was wondering if there was a quick answer @lra.) |
I think the reasoning behind python2 was that this is the version shipped with Mac OS X/macOS But yeah, could be worth thinking about a fork (mackup3 ?) which drops python2 support (but that's something to discuss elsewhere I guess) |
@lra this PR builds on #1143, which seems to have been abandoned? In particular, it:
$HOME
or the current working directory, along with absolute paths./cc @guilhem