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

Improve wording of minimum supported comment #36796

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

jnweiger
Copy link
Contributor

The description for the 'minimum.supported.desktop.version' parameter was misleading. The term 'supported' had multiple meanings here. This PR tries to make the language clearer.

Can we change the name of the parameter to 'minimum.allowed.desktop.version' too? I fear this needs migration steps and has other pitfalls.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

And do:
make test-php-style-fix
to correct the formatting (CI was failing on code style)

config/config.sample.php Outdated Show resolved Hide resolved
config/config.sample.php Outdated Show resolved Hide resolved
@phil-davis phil-davis self-requested a review January 21, 2020 15:07
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

words LGTM

@phil-davis
Copy link
Contributor

@micbar what is the process for changelog with this sort of thing?
Should one be made, and what category?

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/22811/2/6
code style fails - looks like there might be white-space somewhere it is not allowed.

@mmattel
Copy link
Contributor

mmattel commented Jan 21, 2020

Please do a change in the text.

 * Define the minimum ownCloud desktop client version that is allowed to sync with
 * this server instance. ...

Following background:
The very first line of a config value description is used for building the table of contents in docs.
You can imagine that using this current proposal has a quite bad result in docs.

Proposal:

 * Define the minimum ownCloud desktop client version
 * This config value defines the minimum ownCloud desktop client version that is allowed to sync with
 * this server instance. ...

@phil-davis
Copy link
Contributor

@jnweiger ping
Are you progressing this?

@mmattel mmattel force-pushed the jnweiger-clarify-minimum-supported branch from 40dbb8a to d47e88d Compare February 1, 2020 09:17
@mmattel
Copy link
Contributor

mmattel commented Feb 1, 2020

I fixed and squashed the stuff, cleaned up some things additionally.
These were mainly links in rst style which do not work any more (we use antora) and broken links as the content has been removed in docs.

@phil-davis ready to review/merge

@mmattel mmattel force-pushed the jnweiger-clarify-minimum-supported branch from 9951a68 to 1240bda Compare February 1, 2020 09:28
@codecov
Copy link

codecov bot commented Feb 1, 2020

Codecov Report

Merging #36796 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #36796   +/-   ##
=========================================
  Coverage     64.70%   64.70%           
  Complexity    19130    19130           
=========================================
  Files          1270     1270           
  Lines         74868    74868           
  Branches       1329     1329           
=========================================
  Hits          48447    48447           
  Misses        26030    26030           
  Partials        391      391           
Flag Coverage Δ Complexity Δ
#javascript 54.17% <ø> (ø) 0.00 <ø> (ø) ⬆️
#phpunit 65.88% <ø> (ø) 19130.00 <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 022be55...1240bda. Read the comment docs.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

words LGTM

@micbar what is the process for changelog with this sort of thing?

@phil-davis
Copy link
Contributor

@micbar do we need a changelog for this PR?
It is just changing descriptive words in config.sample.php

@phil-davis phil-davis self-requested a review February 4, 2020 10:48
@owncloud owncloud deleted a comment from update-docs bot Feb 4, 2020
@phil-davis phil-davis merged commit 51bd783 into master Feb 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the jnweiger-clarify-minimum-supported branch February 4, 2020 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants