Harmonize true/false usage in return value documentation #858
Conversation
fulldecent
changed the title
|
Dear reviewers. I understand this is many files changed. And please do know that this is a very limited-scope pull request. You may be able to review this quickly by pulling to your local machine, view the full diff and just sort the lines. Or you can run a search on that diff to find and delete lines that are clearly innocuous. For the ambitious, GitHub website has a VIEWED button to track progress on each file you check if you want to get through them on the web interface. If you will ask me to break this up, the most straightforward way is to go module-by-module. But I don't think that will be very helpful. There really are very few kinds of changes in here so please do consider the diff approach above. Thank you! |
|
This is going to be a massive pain for translations, so I'm personally very against this. |
There was a problem hiding this comment.
I haven't reviewed it, but I took a look at a couple of them and I don't think it's a good idea to go ahead with this PR.
Generally, I am in favor of consistency, but in this case I don't really see any gain. Certainly, making all these changes in one go will be challenging. There's a lot to review. There are few types of changes in this PR. It spans across many sections of the manual. You either need to know every function/extension or you can to open the source code to be able to review it properly. This will be very time consuming to do right. Perhaps, this could be done per extension and changes done in separate PRs. e.g. TRUE -> &true; in one PR then falseonfailure in another PR. Making the changes in smaller batches would certainly help in reviewing. Remember that the review needs to be carried out by multiple people probably as there are translators involved who need to review the changes again.
|
I have extracted the Please review and merge that first, it should be the least controversial and easiest to review.. And the justification for why this makes translations more maintainable long-term is included there as well. My plan here is to
|
|
I think this may have a better chance of being approved if the changes were also applied to the translated repos, to reduce workload on translators. Or, have some way to reduce the significant workload that this causes on translators. |
|
None of the changes in this PR are normative on PHP's specification, therefore there is no risk of end-user harm to translations being out-of-sync. As discussed in other comments, the translations are already out of sync, so this does not change the out-of-syncness of the translations. But most importantly, the changes in this PR make the translations significantly easier to maintain long-term. This is because there is much LESS text to translate. Because this helps the translations project long-term and because there is no short-term harm, I recommend that this PR not be blocked on account of translations. That said I might be able to help with translations too, but only after this is all merged. I have added this to my "more notes" file to review at the next iteration. (After existing PRs and fixing broken function specification I identified in another issue.) Because I have already created regexes to help with a lot of this work, I should be able to do a lot of the translators' work too. |
The issue with translation is that you may modify files which are not up to date so you cannot blindly search and replace + update the EN revision tag. Which makes this sort of changes frankly not worth it because it's just a pain for us to review, plus update the translation, especially just before a release were all of the changes and new feature of the release need to be document. At best this should be relegated to be done next year approx February were usually doc works has a bit of downtime. |
|
If I will update the translations then I do not intend to update the revision sync tag. Yes I can blindly change these things. They are formatting changes. In every circumstance in French text where it says true when it needs to be &true; then that is an improvement. If the translation was wrong before I started it's a improvement. If it was right before I started it's an improvement. Just like if I were to update the whitespace in the file or reflow text. There is no need to make a release for that. Or care whether translations were in sync or not. These are simple, stand-alone changes that should not have much controversy to change. If this project will not accept contributions other than in Februaries then this can please be noted in the Readme, helpfully at the top above the title. I am using the PR as a test. It is isolated changes, that barely affect translation, are long-term good, and which nobody disagrees is a fix. If I can't get this merged then I have no hope of considering other changes like where I identified many function which are documented wrong and those updates WILL be normative and WILL require translation. This comment is mostly about the other true/false/null mini-PR and may be less applicable to this larger PR. |
We're not talking about the PR as a whole, but about the huge PR with insignificant changes. There are huge PRs, for example, #623, when the signature has changed - there are no questions for them, these are justified changes. I like this PR, I agree that it will make life easier for translators in the future, but at the moment we don't have the resources to update 600+ files immediately in all active translations. Perhaps splitting this PR into many (very many) small PRs will solve the problem, but that's just my opinion. For other translators: If there'll be a split update, I'm ready to help with French and German translations, if this affects the decision and acceptance/postponement of the PR. UPD: Maybe we'll make a QA-label and group such improvements and merge it from time to time? |
I've never said this is not an improvement, but the ratio improvement/added workload is so terrible that it is not worth to do it at this point in time, especially not in such large PRs which are not contained to one specific extension folder. We've got way more important stuff which should be worked and documented on, especially some things which is going to conflict with your changes, we still need to finish updating the docs to be synced with what the stubs in php-src say, which might remove/amend the return value anyway. We've also still need to finish the migration guide for ValueErrors #163 which has remained a draft and where all of these functions/methods should be updated to reflect that they can throw a ValueError in PHP 8 (and possible emit an E_WARNING in PHP 7). There are also other more important QA work to be done like #648 #649 or just outright documentation writing such as #663 #674 The effort is appreciated, but it is completely misguided into something which is tiresome to revue, doesn't better the overall documentation by that much, and just dumps a shitload of work on translator which are already spread thin. |
|
Again, I don't see why this needs to be translated now at all. If I (with some good reason) wanted to fix the indentation of each file or remove trailing whitespace from lines in the English version, the translators should not be the one to veto that change. If the translators want to fix whitespace in their versions later when available that's fine. And yes it would be more convenient if this PR came when everybody was available, and if everything was coordinated in waterfall fashion, and if everybody got paid to do their job, and if everybody got ice cream. But instead, this PR comes at a time when everybody is busy, just like the other 365 days of the year. So I am asking that the minimum number of people who need to review this (the people working on English version) can judge this on merits and merge (this or the smaller PR for today). |
Because the tool (revcheck) which is used to determine if a translation is up to date checks the commit hash of the English file and compares it with a comment at the top of the file (similar to So any change, even a typo fix, prompts translators to check the file. So yes translators are allowed to veto such changes because it affects the work they need to do. Also no need to get condescending when you don't know the reasons as to why the process is like that, why people have veto rights, and why things work the way they do. Especially because most translators do work on the English docs. Again the work is appreciate, but it is misguided. |
|
I am not trying to exclude translators the people. I am trying exclude translators the concern. I also do translation, but that's not relevant here. So you are describing a deficiency of the tools where it cannot allow a change to marked as minor. Maybe there is a quick fix here. I can take the hash of each file before and after this PR. Then in the translations I can update those hashes (only if matching old hash). End result is if they previously matched they will match after the change. And if they did not match before the change, they were still not match after the change. No additional workload for the translators. Furthermore, we will now have a quick tool that we can use for other minor changes in the future. What do you think? |
|
Yes it is a tooling deficiency, however I don't think your approach is ideal as translators are still going to see a diff for this if the file wasn't up to date when you this change is commited. Ideally there would be a way for the revcheck to skip a commit which is marked with a prefix |
|
@Girgias Thank you, that is super helpful. Implemented. Please find PR here php/web-doc#21 |
|
Okay, good news! If you have been following along only on this larger PR, please see the child PR at #862 We have updated the tooling necessary to make changes like that possible while NOT BOTHERING translators (revcheck is updated). Summary from that child PR:
I am requesting please your review and support on that child PR. When that is done, I can peel it out of this PR, refactor, rebase and spin off another bite sized improvement. |
|
I came to the PHP doc project because I wanted to fix lots of things in the documentation over the past twenty years. Now that the project is on GitHub it is easy (too easy?) to fly by and make my first PR. I chose to first fix the least controversial thing I could find—the formatting and word choice for PHP values This PR handles the problem by completely fixing all affected lines. The other PR handles (part of) the same problem by doing one bite-sized thing. I have also provided detailed notes and offered to handle the same task in translation repositories. I have also contributed a tool change to allow all this to work. One PR was vetoed as being too big a change. The PR other was vetoed as being too small. In a project that has no contributing guide (in the README), and where people exercise veto power in contradictory ways for even a simple change, I see no path forward for me or any new contributor to make significant contributions. With that I am no longer interested in participating here. And I have deleted my fork because it would get very stale between now and next year. Thank you to the people that have reviewed my work so far. |
This PR is not a result of some automatic find and replace. I have manually reviewed hundreds of files to make these changes.
My mission is to review every function return value which is dicsussing
trueandfalsevalues and to harmonize language used when there is a good reason to do so.Reference below is made to some template entities, these are defined in https://github.com/php/doc-en/blob/de9c65c91ff1710d8b2d2ec955caea0162679305/language-snippets.ent
Standardize success boolean text
true/falsesuccess boolean, use standardized textReturns &true; on success, or &false; on failure.Return TRUE on record log information success, FALSE on failure.&return.success;Standardize onto
,&return.falseforfailure;/[ \n,;]*(on\W+success)?[ \n,;]*(or|and|returns)?[ \n,;]*&?false;?\W+\w+\W+(failure|errors?).?/iIf this is a single statement, separated from a complicated explanation, do nothing
Returns &false; on failure.If the return value is describing a bona fide
true/falsesuccess boolean, goto "Standardize success boolean text" aboveIf this is a simple statement describing an expected return value, add a comma, use
&return.falseforfailure;, and remove trailing periodReturns the key on success, or &false; upon failure.Returns the key,&return.falseforfailure;The stored variable or array of variables on success; &false; on failureThe stored variable or array of variables,&return.falseforfailure;If the return value is flat-out wrong, correct it
Performs the virtual command on success, or returns &false; on failure.&return.success;Note: it could be argued in English that every
,&return.falseforfailure;should become;&return.falseforfailure;but that decision is above my paygrade.Format
trueandfalseℹ️ This part of the PR is extracted to #862 for faster review and merge.
/ TRUE | FALSE /And ignore any results inside a
<![CDATA[section.&true;&false;Add
&return.falseproblem;if neededDuring manual review, if there is a function which could return
falseand also return zero or similar, then add&return.falseproblem;Use
&return.nullorfalse;/null\W+on success/iUse
&return.nullorfalse;if appropriate.Use other module-specific template variables as appropriate
Search for the replace-to text of each of the templates in https://github.com/php/doc-en/blob/de9c65c91ff1710d8b2d2ec955caea0162679305/language-snippets.ent to see if using the template entity name is appropriate.
Scope creep
During my manual review I also: