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
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
I just had a quick look, but overall it looks fine :)
kerfuffle/archiveinterface.cpp | ||
---|---|---|
56 | This check should be the first thing in the ctor | |
kerfuffle/cliinterface.h | ||
72–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). | |
86 | Why move this from private to public? | |
105 | Make it protected and add a public getter | |
kerfuffle/cliparameters.h | ||
38–76 ↗ | (On Diff #7839) | 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 ↗ | (On Diff #7839) | Once you drop CliInterface::cacheParameterList() you can probably drop this as well. |
106 ↗ | (On Diff #7839) | unnecessary keyword |
kerfuffle/cliinterface.h | ||
---|---|---|
54–58 ↗ | (On Diff #7910) | 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 | ||
82–110 | 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 ↗ | (On Diff #7910) | 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 ↗ | (On Diff #7908) | 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 ↗ | (On Diff #7908) | Forgot that virtual methods can't be called from the base class ctor. Ignore this. |