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

Remove version checks for < 10 #9676

Merged
merged 2 commits into from
May 17, 2022
Merged

Remove version checks for < 10 #9676

merged 2 commits into from
May 17, 2022

Conversation

TheOneRing
Copy link
Contributor

@TheOneRing TheOneRing commented May 13, 2022

No description provided.

@TheOneRing TheOneRing requested a review from a team May 13, 2022 12:50
src/libsync/account.cpp Show resolved Hide resolved
// Older version which is not "end of life" according to https://github.com/owncloud/core/wiki/Maintenance-and-Release-Schedule
return serverVersionInt() < makeServerVersion(10, 0, 0) || serverVersion().endsWith(QLatin1String("Nextcloud"));
return serverVersion() < QVersionNumber(10) || serverProduct().endsWith(QLatin1String("Nextcloud"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Readability could be improved by moving the Nextcloud check into a dedicated if.

}

void Account::setServerVersion(const QString &version)
void Account::setServerVersion(const QString &version, const QString &productName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This setter's purpose has changed, so it should likely be renamed. There are two dedicated getters for both the version and the product name anyway.

@@ -168,23 +168,18 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject

bool hasCapabilities() const;

// product
QString serverProduct() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about serverProductName?

The comment is redundant and doesn't add information, so it should be removed again.

static bool envEnabled = [] {
const auto env = qEnvironmentVariable("OWNCLOUD_PARALLEL_CHUNK");
if (!env.isEmpty()) {
return env != QLatin1String("false") && env != QLatin1String("0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe allow any arbitrary casing of the value by comparing to a lowercase value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The env var is not even documented and its behavior is orthogonal to all other env checks.
Unrelated to the review

@TheOneRing TheOneRing force-pushed the work/no_version_check branch 2 times, most recently from a9ceb55 to 21beea0 Compare May 17, 2022 08:09
@@ -312,13 +315,12 @@ void ConnectionValidator::fetchUser()
job->start();
}

bool ConnectionValidator::setAndCheckServerVersion(const QString &version)
bool ConnectionValidator::setAndCheckServerVersion(const QString &version, const QString &serverProduct)
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming wise you may want to call this method setAndCheckServerInfo now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@TheOneRing TheOneRing force-pushed the work/no_version_check branch from 21beea0 to a089025 Compare May 17, 2022 08:31
@TheOneRing TheOneRing merged commit bddc2ae into master May 17, 2022
@delete-merged-branch delete-merged-branch bot deleted the work/no_version_check branch May 17, 2022 08:53
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing linked an issue May 18, 2022 that may be closed by this pull request
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.

Remove version dependen checks and only rely on capabilities
3 participants