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

Global const for KeyFileOption (CLI) #2505

Merged
merged 2 commits into from
Nov 28, 2018

Conversation

louib
Copy link
Member

@louib louib commented Nov 23, 2018

Motivation and context

Same option is defined in most of the CLI commands. Also, more global options might appear in the future (--no--password, --silent, --yubikey, for example).

How has this been tested?

unit tests.

Types of changes

  • ✅ Refactoring

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]

@louib louib requested review from droidmonkey and a team and removed request for droidmonkey November 23, 2018 21:58
@louib louib added this to the v2.4.0 milestone Nov 28, 2018
@louib
Copy link
Member Author

louib commented Nov 28, 2018

@droidmonkey rebased this one after merging #2507

@droidmonkey
Copy link
Member

After rebasing this, I realized there should really be a common method to opening the database within the CLI. We are repeating the call to

auto db = Database::unlockFromStdin(databasePath,
                                        parser.value(Command::KeyFileOption),
                                        parser.isSet(Command::QuietOption) ? Utils::DEVNULL : Utils::STDOUT,
                                        Utils::STDERR);

Many many times with the same inputs.

@phoerious
Copy link
Member

That method needs to be refactored out of the Database class anyway.

@droidmonkey droidmonkey merged commit 49b82ea into keepassxreboot:develop Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants