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

Feature: Config class for getting installation config without running the installation process #39

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

Conversation

vonNiklasson
Copy link

@vonNiklasson vonNiklasson commented Mar 29, 2021

Adds a separate class called InstallConfiguration which holds the actual configuration for auto completion installation. The reason for this refactoring and feature was to add a way to get the path to the shell config without actually having to run to installation process.

Backwards compatibility is preserved while adding the possibility to install with the configuration class as well through the new function install_from_config(install_config: InstallConfiguration) -> Tuple[str, str].

Also added documentation info Return and Raises for the install function.

Example usage with the new class:

import click_completion.core

# Creates an InstallConfiguration class. This takes all parameters that the `install` function takes.
install_config = click_completion.core.InstallConfiguration()
# Run the actual installation process from the InstallConfiguration class
click_completion.core.install_from_config(install_config)

Comment on lines +407 to +412
if append is not None:
mode = 'a' if append else 'w'
else:
mode = mode

return path, mode
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fully aware that this overrides the previous configuration setting on the lines above, but since this was the behaviour of the old way I don't want to break it with this PR.

@Konubinix
Copy link
Collaborator

LGTM. I would have put the whole logic into the class, even the install method. And then I would have called that an installer. But in its current state it looks ok anyway.

@glehmann : van you see a reason not to merge this upstream?

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.

None yet

2 participants