Use &true; &false; &null; semantically where appropriate by fulldecent · Pull Request #862 · php/doc-en

fulldecent · 2021-08-18T14:28:27Z

This PR is not a result of some automatic find and replace. I have manually reviewed hundreds of files to make these changes.

Format true and false

/ TRUE | FALSE | NULL /i

and ignore any results inside a <![CDATA[ section or where "true"/"false" have meanings other than the boolean types inside PHP.

If a true value, use &true;
If a false value, use &false;
If a null value, use &null;

Why

Improve readability. Make long-term maintenance of translations easier.

Effect on translations

The true, false and null values are semantic and this should help with the long-term maintenance of translations.

Translators will not need to think much about whether true means "真正的布尔值" or "真不变量" when seeing:

true color image
return value of &true;
this invariant is true if X and Y
null byte
null-terminated

because I have already separated the cases of true and &true;.

Scope creep

This PR also fixes two typos of true is to become true if immediately on the lines a was editing.

kamil-tekiela

I am not against the idea of changing the values. I see the value in it, but I still believe this PR is too big. This is 288 files to carefully be reviewed.
While I understand that you only want to change this single thing, but your PR touches pages that haven't been maintained in a long time. There are very few volunteers who maintain PHP manual in their free time. While doing this change, I feel that we should try to address as many issues as we see on the lines that are being edited. This means that there will be related PRs that will try to fix issues we find (I just submitted one PR). This will make reviewing this PR take a lot longer. Some issues, which are out of the scope of this PR, might even be added to this PR, bloating it even more.

I don't know what other more experienced contributors think, but if you could split this PR into even smaller chunks then it would help with the workload. If not, then please expect this PR to take a while before it is reviewed properly.

kamil-tekiela · 2021-08-18T14:49:43Z

     <link linkend="object.construct">__construct()</link> and
     <link linkend="object.destruct">__destruct()</link> methods of interfaces.
-     Previously, this would only be true for methods of classes and traits.
+     Previously, this would only be &true; for methods of classes and traits.


I think that was mean to be the word "true", not the constant TRUE.

Disagree. Here is the same text without confusing markup:

... now also return &true; for __construct() and __destruct() methods of interfaces. Previously, this would only be &true; for methods of classes and traits.

Maybe "Previously, this would only be the case for methods of classes and traits." would be better.

I have a "more notes" file where I am tracking this specific suggestion.

I hope that the merging of the improvements in these 300 changes I am proposing can continue without requiring the further incremental changes proposed here and below.

We agree that your note is good. We agreed it should be done. I'm noting it. I'll do it. But I don't see anything here which says that that this PR should be considered broken or blocked.

kamil-tekiela · 2021-08-18T15:04:29Z

  &reftitle.returnvalues;
  <para>
-   Shall return true if this Definition is registered
+   Shall return &true; if this Definition is registered


Is "shall" the right word here? What is "Definition"? Shouldn't this be the reference to the class name?

I have a "more notes" file where I am tracking this specific suggestion.

I hope that the merging of the improvements in these 300 changes I am proposing can continue without requiring the further incremental changes proposed here and below.

We agree that your note is good. We agreed it should be done. I'm noting it. I'll do it. But I don't see anything here which says that that this PR should be considered broken or blocked.

cmb69 · 2021-08-18T15:26:24Z

I think the decision whether it is good to make this change without any unrelated changes should be made by the translators. @saundefined, @Girgias, @mumumu, what do you think?

fulldecent · 2021-08-18T15:34:09Z

Thank you for the feedback so far.

I have ambitions to make a lot of changes here. And there is a lot of stuff here that needs changes and other work to do.

My experience is from other large, volunteer-run projects with not enough resources, such as the KDE project of decades ago. I am hoping to make changes and get forward progress here. The best way is if I can prove that I'm not breaking anything and that if something is broken then it is really rare and not hurting anybody, but lost in a sea of other improvements.

Ideally I want to borrow authority to make these changes. If all my changes need to be reviewed by every code owner (living or dead), or if everything I am touching will need to meet more metrics of correctness than are in scope of this a given PR then my efforts will fail.

There are ~300 lines of code changed here. And the best way to review these is using git diff, likely only with one coffee. I really think this specific PR can be approved by one person using common sense without relying on other people to confirm the formatting requirements for the true/false/null values.

My plan is to make this merge and then immediately release the next bite-sized PR from #858. This will overwrite some of the changes made in this PR, but the benefit is that it should be very easy to review and merge and that separate change can be judged on its own merits.

saundefined · 2021-08-18T16:02:25Z

There are ~300 lines of code changed here. And the best way to review these is using git diff, likely only with one coffee. I really think this specific PR can be approved by one person using common sense without relying on other people to confirm the formatting requirements for the true/false/null values.

Thanks for your hard work!

Many translations now lagging behind the English version (fr 258 files, de — 375),
and I'm afraid that these minor changes will set translations back even further.

You can see the comment for similar minor changes.

I agree that sometime this needs to be done, but I'm not sure if now.

fulldecent · 2021-08-18T16:27:15Z

Long-term, this change helps make translations easier to manage.

And furthermore, the next change coming after this PR makes translations much easier. It significantly reduces the number of things that need to be translated, for all languages.

Because the translations are already not up-to-date (i.e. short-term), and because this and my next PR will make translations easier to maintain (i.e. long-term), I hope that translations impact are not a blocker for merging this PR.

fulldecent · 2021-09-10T01:09:41Z

Previously, this commit was at risk of spamming all the translators with changes on the revcheck dashboard that were useless, because this is a cosmetic change for the English docs that does not require any action from translators.

We have updated and deployed a change to revcheck called [skip-revcheck] (the square brackets ARE significant). Any commit starting with that text will be excluded from the revcheck tool.

This PR uses that feature. In addition I have rebased and squashed.

End result: this is one commit in one PR that changes one thing (formatting of true/false/null) on the English docs and should not have any impact whatsoever on translators.

salathe · 2021-09-10T08:06:56Z

   The <function>cubrid_errno</function> function is used to get the
   error code of the error that occurred during the API execution.  Usually,
-   it gets the error code when API returns false as its return value.
+   it gets the error code when API returns &false;.


Nit (consistency): as its return value is removed here, but not from the several other places where the same occurs. Your preference is to not include this kind of change in this PR, so I suggest reverting the removal of "as its return value" here.

salathe · 2021-09-10T08:08:17Z

   <listitem>
    <simpara>
-     Use NULL instead of FALSE on failure.
+     Use NULL instead of &false; on failure.


Suggested change

Use NULL instead of &false; on failure.

Use &null; instead of &false; on failure.

salathe · 2021-09-10T08:13:46Z

   from the server. The callback is passed the Memcached object, the requested
   key, and the by-reference value variable. The callback is responsible for
-   setting the value and returning true or false. If the callback returns true,
+   setting the value and returning &true; or false. If the callback returns true,


Suggested change

setting the value and returning &true; or false. If the callback returns true,

setting the value and returning &true; or &false;. If the callback returns &true;,

salathe · 2021-09-10T08:25:42Z

  &reftitle.returnvalues;
  <para>
-   Returns TRUE if the offset exists, otherwise FALSE.
+   Returns &true; if the offset exists, otherwise FALSE.


Suggested change

Returns &true; if the offset exists, otherwise FALSE.

Returns &true; if the offset exists, otherwise &false;.

salathe · 2021-09-10T08:25:53Z

  &reftitle.returnvalues;
  <para>
-   Returns TRUE if the current iterator position is valid, otherwise FALSE.
+   Returns &true; if the current iterator position is valid, otherwise FALSE.


Suggested change

Returns &true; if the current iterator position is valid, otherwise FALSE.

Returns &true; if the current iterator position is valid, otherwise &false;.

salathe · 2021-09-10T08:37:18Z

@@ -32,7 +32,7 @@
    <listitem>
     <para>
      Callback function to invoke when the device is idle. Returning false


Suggested change

Callback function to invoke when the device is idle. Returning false

Callback function to invoke when the device is idle. Returning &false;

salathe · 2021-09-10T08:37:39Z

@@ -32,7 +32,7 @@
    <listitem>
     <para>
      Callback function to invoke when the device is idle. Returning false


Suggested change

Callback function to invoke when the device is idle. Returning false

Callback function to invoke when the device is idle. Returning &false;

salathe · 2021-09-10T08:46:48Z

 <refsect1 role="returnvalues">
  &reftitle.returnvalues;
-  <para>Returns an array with calculated data or false on failure.</para>
+  <para>Returns an array with calculated data or &false; on failure.</para>


This text is repeated over 150 times for other trader functions. Suggestion (not in this PR): make it an entity in language-snippets.ent. Note: doing so will break builds for any translations that have not translated the file and not added the entity.

kamil-tekiela

I have been reviewing for an hour and I am only 1/3 of the way through.

kamil-tekiela · 2021-09-10T11:12:03Z

   from the server. The callback is passed the Memcached object, the requested
   key, and the by-reference value variable. The callback is responsible for
-   setting the value and returning true or false. If the callback returns true,
+   setting the value and returning &true; or false. If the callback returns true,


Suggested change

setting the value and returning &true; or false. If the callback returns true,

setting the value and returning &true; or &false;. If the callback returns &true;,

kamil-tekiela · 2021-09-10T11:15:42Z

      <listitem>
        <para>
-          <code>required</code>: bool, (optional) true if the field is required to exist in the document. 
+          <code>required</code>: bool, (optional) &true; if the field is required to exist in the document. 


Suggested change

<code>required</code>: bool, (optional) &true; if the field is required to exist in the document.

<code>required</code>: &boolean;, (optional) &true; if the field is required to exist in the document.

kamil-tekiela · 2021-09-10T11:22:27Z

    <term><parameter>destroy</parameter></term>
    <listitem>
-     <para>When true pthreads will destroy the Mutex after a successful unlock.</para>
+     <para>When &true; pthreads will destroy the Mutex after a successful unlock.</para>


Suggested change

<para>When &true; pthreads will destroy the Mutex after a successful unlock.</para>

<para>When &true;, pthreads will destroy the Mutex after a successful unlock.</para>

kamil-tekiela · 2021-09-10T11:26:00Z

            </para>
            <para>
-              $allowDups = false is the same as $overwrite = true
+              $allowDups = &false; is the same as $overwrite = &true;


This should be rewritten to use parameter names (separate PR). Don't change this now.

kamil-tekiela · 2021-09-10T11:26:31Z

  &reftitle.returnvalues;
  <para>
-   Returns a string on success and null if there is nothing to return.
+   Returns a string on success and &null; if there is nothing to return.


Suggested change

Returns a string on success and &null; if there is nothing to return.

Returns a string on success or &null; if there is nothing to return.

It doesn't return both values at the same time.

kamil-tekiela · 2021-09-10T11:28:07Z

  &warn.undocumented.func;
  <para>
-   Deletes a file or a directory, return true if all is ok, false otherwise
+   Deletes a file or a directory, return &true; if all is ok, &false; otherwise


Suggested change

Deletes a file or a directory, return &true; if all is ok, &false; otherwise

Deletes a file or a directory, returns &true; if all is ok, &false; otherwise

kamil-tekiela · 2021-09-10T11:28:19Z

 <refnamediv>
  <refname>svn_fs_delete</refname>
-  <refpurpose>Deletes a file or a directory, return true if all is ok, false otherwise</refpurpose>
+  <refpurpose>Deletes a file or a directory, return &true; if all is ok, &false; otherwise</refpurpose>


Suggested change

<refpurpose>Deletes a file or a directory, return &true; if all is ok, &false; otherwise</refpurpose>

<refpurpose>Deletes a file or a directory, returns &true; if all is ok, &false; otherwise</refpurpose>

kamil-tekiela · 2021-09-10T11:29:57Z

  </methodsynopsis>
  <para>
-    If $remove is set to be true and $offset is set to be 0, the data will be removed from the buffer. 
+    If $remove is set to be &true; and $offset is set to be 0, the data will be removed from the buffer. 


For future PR: parameter names should be using <parameter> tags instead of PHP syntax.

salathe · 2021-09-10T13:30:27Z

TL;DR – I am vetoing this change.

What bothers me most about this particular PR, is that many (most?) of the changes will be changed again soon, based on the plan in the parent PR #858. Further changes, such as adding lots and lots of &return.falseforfailure;, adding an entity for the 150+ identical return values for the trader functions, etc.. Even if this PR is applied, there will be further sweeping changes requested to the same lines changed in this PR.

I also do not share the author's view point that this PR "should not have any impact whatsoever on translators". Impacting translators is spelled out as the main point of this PR. Translators are expected to pick up the changes in this PR (and all changes coming from the parent PR), otherwise what is the point? To that end, the use of the [skip-revcheck] tag in the commit message is inappropriate.

Because of the above, and the comments from other authors and translators in this PR and the parent PR that I won't repeat here, I don't think it is worth our (the author, reviewers, translators) while to continue with this PR.

Where to go from here? The experiment failed. @fulldecent and interested parties, let us regroup (in the parent PR, or a new Issue perhaps) and consider alternative directions to approach this from.

fulldecent changed the title ~~Use &true; and &false; semantically where appropriate~~ Use &true; &false; &null; semantically where appropriate Aug 18, 2021

fulldecent mentioned this pull request Aug 18, 2021

Harmonize true/false usage in return value documentation #858

Closed

kamil-tekiela reviewed Aug 18, 2021

View reviewed changes

[skip-revcheck] Use &true; and &false; semantically where appropriate

4d19998

fulldecent force-pushed the semantic-true-false branch from 2be7d36 to 4d19998 Compare September 10, 2021 00:59

salathe added the mahoosive-change Grab a cup of coffee and dive in. label Sep 10, 2021

salathe requested changes Sep 10, 2021

View reviewed changes

kamil-tekiela requested changes Sep 10, 2021

View reviewed changes

salathe closed this Sep 10, 2021

kamil-tekiela mentioned this pull request Sep 10, 2021

Tidy up SVN docs #939

Merged

	Use NULL instead of &false; on failure.
	Use &null; instead of &false; on failure.

	setting the value and returning &true; or false. If the callback returns true,
	setting the value and returning &true; or &false;. If the callback returns &true;,

	Returns &true; if the offset exists, otherwise FALSE.
	Returns &true; if the offset exists, otherwise &false;.

	Returns &true; if the current iterator position is valid, otherwise FALSE.
	Returns &true; if the current iterator position is valid, otherwise &false;.

	Callback function to invoke when the device is idle. Returning false
	Callback function to invoke when the device is idle. Returning &false;

	<code>required</code>: bool, (optional) &true; if the field is required to exist in the document.
	<code>required</code>: &boolean;, (optional) &true; if the field is required to exist in the document.

	<para>When &true; pthreads will destroy the Mutex after a successful unlock.</para>
	<para>When &true;, pthreads will destroy the Mutex after a successful unlock.</para>

	Returns a string on success and &null; if there is nothing to return.
	Returns a string on success or &null; if there is nothing to return.

	Deletes a file or a directory, return &true; if all is ok, &false; otherwise
	Deletes a file or a directory, returns &true; if all is ok, &false; otherwise

	<refpurpose>Deletes a file or a directory, return &true; if all is ok, &false; otherwise</refpurpose>
	<refpurpose>Deletes a file or a directory, returns &true; if all is ok, &false; otherwise</refpurpose>

Uh oh!

Conversation

fulldecent commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Format true and false

Why

Effect on translations

Scope creep

Uh oh!

kamil-tekiela left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cmb69 commented Aug 18, 2021

Uh oh!

fulldecent commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saundefined commented Aug 18, 2021

Uh oh!

fulldecent commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fulldecent commented Sep 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salathe commented Sep 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

fulldecent commented Aug 18, 2021 •

edited

Loading

fulldecent commented Aug 18, 2021 •

edited

Loading

fulldecent commented Aug 18, 2021 •

edited

Loading