Skip to content

Refactoring : Command base class for CLI commands#778

Merged
louib merged 1 commit intokeepassxreboot:developfrom
louib:refactoring_command_class
Jul 17, 2017
Merged

Refactoring : Command base class for CLI commands#778
louib merged 1 commit intokeepassxreboot:developfrom
louib:refactoring_command_class

Conversation

@louib
Copy link
Copy Markdown
Member

@louib louib commented Jul 16, 2017

This PR introduces a Command class, from which all the CLI commands inherit. This makes it easy to programmatically get the names of all the available commands, using the factory-like static functions in the Command class.

Also took this opportunity to add most of the information on an entry to the show command, and renamed the list command to ls, to fit the unix name.

Motivation and context

This will make it easier to add new cli commands (there will be more in the future...)

How has this been tested?

  • unit tests are passing
  • Tested manually that all the commands were still working.

Types of changes

  • ✅ Breaking change
  • ✅ 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]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • ✅ My change requires a change to the documentation and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

@louib louib requested a review from a team July 16, 2017 17:27
@louib louib force-pushed the refactoring_command_class branch 3 times, most recently from 563c01c to 1d3b811 Compare July 16, 2017 17:41
@louib louib force-pushed the refactoring_command_class branch from 1d3b811 to e946644 Compare July 16, 2017 17:45
@louib louib changed the title Refactoring : Introducing Command class for CLI commands Refactoring : Command base class for CLI commands Jul 16, 2017
@louib
Copy link
Copy Markdown
Member Author

louib commented Jul 17, 2017

@TheZ3ro can you take a look at this one? It adresses some points in #681 and I'd like to get it merged before adding new commands to the CLI.

parser.showHelp(EXIT_FAILURE);
}

int exitCode = command->execute(argc, argv);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about removing argv[0] (keepassxc) and lower argc by 1 here instead of doing it in every Command?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@TheZ3ro the argv and argc have to be intact when instantiating the QApplication, otherwise the name of the app will be incorrect. I plan to only use QCoreApplication in the CLI in the future, so that we can instantiate it once in keepassxc-cli. This will require:
1- Using QProcess + native cli apps for clipping (already in progress)
2- Removing the gui prompt in favor of supporting a --key-file argument in all the CLI commands.

Until then, we have to rely on duplicating the code in the commands...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So It's ok 👍

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Jul 17, 2017

EntropyMeter need some love but the PR is ok, just take a look at the note above

@louib louib merged commit 3b23e68 into keepassxreboot:develop Jul 17, 2017
@louib louib deleted the refactoring_command_class branch July 17, 2017 19:16
varjolintu pushed a commit to varjolintu/keepassxc that referenced this pull request Sep 10, 2017
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.

2 participants