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

doc: fix typos in config.rst #16681

Merged
merged 1 commit into from Aug 1, 2017

Conversation

Projects
None yet
5 participants
@shun-s
Contributor

shun-s commented Jul 30, 2017

Fix several typos in config.rst

Signed-off-by: Song Shun song.shun3@zte.com.cn

@joscollin joscollin changed the title from doc: fix sevral typos to doc: fix typos in config.rst Jul 30, 2017

New-style config options are defined in common/options.cc. All new config
options should go here (and not into legacy_config_opts.h).
New-style config options are defined in common/options.cc. All new config
options should go here (and not into legacy config_opts.h).

This comment has been minimized.

@joscollin

joscollin Jul 30, 2017

Member

This seems to be wrong. There is a file src/common/legacy_config_opts.h

This comment has been minimized.

@shun-s

shun-s Jul 31, 2017

Contributor

yeah, that's true.
already recovery to legacy_config_opts.h.
thanks

@joscollin

Looks Good.

@joscollin

This comment has been minimized.

Member

joscollin commented Jul 31, 2017

Jenkins retest this please

doc: fix sevral typos
  fix sevral typos

Signed-off-by: Song Shun <song.shun3@zte.com.cn>
@joscollin

This comment has been minimized.

Member

joscollin commented Jul 31, 2017

Jenkins retest this please

@ceph-jenkins

This comment has been minimized.

Collaborator

ceph-jenkins commented Jul 31, 2017

make check failed

@joscollin

This comment has been minimized.

Member

joscollin commented Jul 31, 2017

Jenkins retest this please

@joscollin joscollin merged commit 7e9d87f into ceph:master Aug 1, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
@@ -57,7 +57,7 @@ Reading configuration values
There are two ways for Ceph code to get configuration values. One way is to
read it directly from a variable named "g_conf," or equivalently,
"g_ceph_ctx->_conf." The other is to register an observer that will be called
every time the relevant configuration values changes. This observer will be
every time the relevant configuration values changes. This observer will be

This comment has been minimized.

@tchaikov

tchaikov Aug 1, 2017

Contributor

@shun-s @joscollin why is this considered a typo? is this an accepted guide line to have a single space to start a new sentence in our document? or is it recommended by some best practice ? if yes, for what reason?

for more counter examples, see https://github.com/ceph/ceph/pull/16611/files

This comment has been minimized.

@joscollin

joscollin Aug 1, 2017

Member

@tchaikov I found that these minor changes looks better along with other typos that are fixed by @shun-s in this PR. The mistake I did was: somehow I missed the legacy_config_opts.h revert. Created a new PR for that: #16721.

This comment has been minimized.

@tchaikov

tchaikov Aug 1, 2017

Contributor

@joscollin my questions still hold.

This comment has been minimized.

@tchaikov

tchaikov Aug 1, 2017

Contributor

@joscollin @shun-s i'd suggest avoid these sort of "minor changes" if possible unless they are piggy-backed by more significant changes. IMO, they are not typo at all. and there are not visible change introduced by removing the spaces. please try to build the script and check the output.

This comment has been minimized.

@joscollin

joscollin Aug 1, 2017

Member

@tchaikov Yes, I completely accept your suggestion. But there were other significant changes in this PR along with these minors.

This comment has been minimized.

@tchaikov

tchaikov Aug 1, 2017

Contributor

like the comma removals?

This comment has been minimized.

@joscollin

joscollin Aug 1, 2017

Member

@tchaikov Yes, they were unnecessary. Looks like a copy-paste mistake with those options.

@ZVampirEM77

This comment has been minimized.

Contributor

ZVampirEM77 commented on doc/dev/config.rst in 2556605 Aug 1, 2017

I think legacy_config_opts.h is OK

@joscollin

This comment has been minimized.

Member

joscollin commented Aug 1, 2017

@ZVampirEM77 Before merging, I have verified that the change made for legacy_config_opts.h doesn't appear in the Files Changed tab any more and reverted. But for some reason the wrong text was merged. I will create a new PR now to fix it. Thanks for the information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment