feat(cli): support multiple folders to sync with --include file - #10574
feat(cli): support multiple folders to sync with --include file#10574Arne-Zillhardt wants to merge 1 commit into
--include file#10574Conversation
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
sorry for the delay @Arne-Zillhardt
seems like we should get some new automated tests to ensure lack of regressions
I did not test your changes but they seem to me like the correct way to solve your use case
can you include automated tests that would use the simple way (without --include) and the new way of operating the command line client ?
maybe you could do them in a similar way to https://github.com/nextcloud/desktop/blob/master/test/testnextcloudcmdprovisioning.cpp
| options->source_dir = fi.absoluteFilePath(); | ||
| } | ||
| options->source_dir = fi.absoluteFilePath(); | ||
|
|
There was a problem hiding this comment.
can you remove the extra while line ?
There was a problem hiding this comment.
I think I remove all extra white lines now? I had to rebase on master because of other changes in src/cmd/cmd.cpp (I think you also made them xD). I hope I merged them correcty.
I also (more or less) copied from the provisioning test and added a test for the --include flag. I hope the test cases are the right ones
| } else if (option == "--include" && !it.peekNext().startsWith("-")) { | ||
| options->include = it.next(); | ||
| QFileInfo fi(options->include); | ||
| if (!fi.exists()) { | ||
| std::cerr << "Include file '" << qPrintable(options->include) << "' does not exist." << std::endl; | ||
| exit(1); | ||
| } | ||
| options->include = fi.absoluteFilePath(); |
There was a problem hiding this comment.
for simplicity, I would enforce the use of a single occurrence of the new option --include
so can you test that options->include is empty and if not return an error and do not proceed
There was a problem hiding this comment.
I don't exactly know what you mean, but now --include only gets checked before if it is included, before parsing the file
| QMap<QString, QString> foldersToSync; | ||
| if (!options.source_dir.isEmpty()) { | ||
| foldersToSync.insert(options.source_dir, options.remotePath); | ||
| } else { | ||
| QFile file(options.include); | ||
| if (!file.open(QIODevice::ReadOnly)) { | ||
| std::cerr << "Include file '" << qPrintable(file.fileName()) << "' could not be opened" << std::endl; | ||
| exit(1); | ||
| } | ||
|
|
||
| while (!file.atEnd()) { | ||
| QByteArray line = file.readLine().trimmed(); | ||
|
|
||
| if (line.isEmpty() || line.startsWith('#')) { | ||
| continue; | ||
| } | ||
|
|
||
| QStringList splitEntry = QString::fromUtf8(line).split(u' '); | ||
| if (splitEntry.count() != 2) { | ||
| std::cerr << "Entry '" << qPrintable(QString::fromUtf8(line)) << "' is not in format '<local path> <remote path>'" <<std::endl; | ||
| exit(1); | ||
| } | ||
|
|
||
| QString localPath = splitEntry.at(0); | ||
| if (localPath.endsWith('/')) { | ||
| localPath.append('/'); | ||
| } | ||
|
|
||
| QFileInfo fi(localPath); | ||
| if (!fi.exists()) { | ||
| std::cerr << "Included dir '" << qPrintable(localPath) << "' does not exist." << std::endl; | ||
| exit(1); | ||
| } | ||
|
|
||
| foldersToSync.insert(fi.absoluteFilePath(), splitEntry.at(1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
can you spit that into an utility function doing the parsing ?
in addition, we aim to use auto almost everywhere
can you try using it more ?
for example:
const auto splitEntry = QString::fromUtf8(line).split(u' ');
There was a problem hiding this comment.
I made a utility function parseIncludeFile and hopefully used auto in my changes :)
Signed-off-by: Arne-Zillhartd <arne-zillhardt@noreply.codeberg.org>
079069e to
f129671
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
2 participants
Resolves
#4780
Summary
Added the new
--includeflag that reads a file with the format<local path> <remote path>and syncs these folders with nextcloud.I'm not really confident or sure about the implementation. I don't know if it's a good idea to just recreate the SyncEngine and iterate over the list of directorys (maybe also replace
~with the user home?). I think the best and cleanest solution would be to give SyncEngine not only a list of files to exclude, but also to include, that it doesn't need to be recreated every time. But I'm not really sure how big of a change that would be or if it is even worth it.So this is the proposal for a solution to the issue :)
TODO
Checklist
AI (if applicable)