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

ensure SyncEngine use an initialized instance of SyncOptions #4816

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

mgallien
Copy link
Collaborator

@mgallien mgallien commented Aug 4, 2022

will prevent nextcloudcmd command line client from ignoring the settings
handled by SyncOptions

Signed-off-by: Matthieu Gallien matthieu.gallien@nextcloud.com

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Please see comment

@mgallien mgallien force-pushed the bugfix/ensureSyncOptionsAreInitialized branch from c578587 to 48d4b24 Compare August 4, 2022 14:53
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #4816 (8551a14) into master (2d8fb14) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4816      +/-   ##
==========================================
+ Coverage   56.79%   57.22%   +0.43%     
==========================================
  Files         138      138              
  Lines       17143    17144       +1     
==========================================
+ Hits         9736     9811      +75     
+ Misses       7407     7333      -74     
Impacted Files Coverage Δ
src/libsync/syncengine.h 43.75% <ø> (ø)
src/libsync/syncengine.cpp 87.22% <100.00%> (+0.02%) ⬆️
src/libsync/vfs/cfapi/cfapiwrapper.cpp 74.71% <0.00%> (+0.45%) ⬆️
src/libsync/theme.cpp 12.67% <0.00%> (+0.60%) ⬆️
src/libsync/propagateremotemkdir.cpp 65.24% <0.00%> (+0.70%) ⬆️
src/libsync/configfile.cpp 25.73% <0.00%> (+9.04%) ⬆️
src/libsync/syncoptions.cpp 65.71% <0.00%> (+45.71%) ⬆️

Copy link
Contributor

@allexzander allexzander left a comment

Choose a reason for hiding this comment

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

Looks nice. @mgallien @claucambra I'd say, we need to have 2 constructors for SyncEngine ideally, such that one that does not take SyncOptions as a parameter will always do the initialization job inside, and the other one will be used when we need to pass empty SyncOptions.
Just an idea for refactoring.

@allexzander allexzander requested a review from claucambra August 8, 2022 06:25
will prevent nextcloudcmd command line client from ignoring the settings
handled by SyncOptions

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@allexzander allexzander force-pushed the bugfix/ensureSyncOptionsAreInitialized branch from 48d4b24 to 8551a14 Compare August 8, 2022 09:36
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4816-8551a14c48bbfac15785a4cc00ab19aa9fc46f3e-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 8, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

50.0% 50.0% Coverage
0.0% 0.0% Duplication

@allexzander allexzander merged commit dab1a19 into master Aug 8, 2022
@allexzander allexzander deleted the bugfix/ensureSyncOptionsAreInitialized branch August 8, 2022 12:02
@mgallien mgallien added this to the 3.6.0 milestone Sep 2, 2022
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.

4 participants