Skip to content
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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

patrickmckenna
Copy link

@lra this PR builds on #1143, which seems to have been abandoned? In particular, it:

  • Adds validation logic for the user-supplied path, per the request in Add ability to specify a custom configuration file #1143 (comment). (The goal was a more robust check, that the file actually exists, instead of just simple type checking.)
  • Automatically handles paths given relative to $HOME or the current working directory, along with absolute paths.
  • Adds tests of the new behavior, and confirming that the existing default behavior is preserved.

/cc @guilhem

@patrickmckenna
Copy link
Author

@lra friendly bump 😄

@lra
Copy link
Owner

lra commented Jan 30, 2019

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 ;)

@patrickmckenna
Copy link
Author

@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.

Copy link
Contributor

@tillhainbach tillhainbach left a 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

@patrickmckenna
Copy link
Author

 @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.)

@tillhainbach
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants