CliInterface is refactored by creating a new class (CliParameters) which stores and handles all commands, switches and regexp patterns used for interacting with the cli executables.
Details
- Reviewers
elvisangelaccio - Commits
- R36:aaed7270b611: CliInterface refactoring
R36:1074bf07a9b9: Refactor CliInterface
R36:fc25a20d533f: Implement Elvis' suggestions.
R36:826d0993f9c4: Merge branch 'cli-refactor' of git.kde.org:ark into cli-refactor
R36:4141c3833470: Merge branch 'master' into cli-refactor
Diff Detail
- Repository
- R36 Ark
- Branch
- cli-refactor
- Lint
No Linters Available - Unit
No Unit Test Coverage
I just had a quick look, but overall it looks fine :)
kerfuffle/archiveinterface.cpp | ||
---|---|---|
54 | This check should be the first thing in the ctor | |
kerfuffle/cliinterface.h | ||
73 | This method in not really necessary. Every plugin could just do the setup in their own constructor (since they have access to m_cliParameters). | |
87 | Why move this from private to public? | |
116 | Make it protected and add a public getter | |
kerfuffle/cliparameters.h | ||
38–76 | Since this new class is not only about parameters/switches but also other CLI stuff (regexes, etc.) maybe we should find a more descriptive name, though I can't think any at the moment :p | |
81 | Once you drop CliInterface::cacheParameterList() you can probably drop this as well. | |
106 | unnecessary keyword |
kerfuffle/cliinterface.h | ||
---|---|---|
54–58 | Maybe we should keep the docs in this enum. You can move them above the Q_PROPERTY they end up in. | |
146 | This method can be moved back to the private section. | |
kerfuffle/cliproperties.h | ||
81–109 ↗ | (On Diff #7874) | All the methods of the new class can be marked as const , it seems. |
- Make substitute* functions in CliProperties class const.
- Make CliInterface::escapeFileName() private again.
kerfuffle/cliinterface.h | ||
---|---|---|
54–58 | I would rather not keep the doc. It reduces the readability of the code and most of the properties are pretty self-explanatory. |
plugins/cliunarchiverplugin/cliplugin.h | ||
---|---|---|
58 | One more thing: since all cli plugins need this method, make it virtual and call in just once in the CliInterface ctor |
plugins/cliunarchiverplugin/cliplugin.h | ||
---|---|---|
58 | Forgot that virtual methods can't be called from the base class ctor. Ignore this. |