tools: fix incorrect version history order #45728

welfoz · 2022-12-03T19:55:48Z

Hi!

This is the first PR of my life 💯
I tried my best 🚀

Please tell me if this fix needs a test or not.

Here is my commit message:
This fixes an error in parseYAML(text), the version sorting coudn't be right as we compared an arrify string (ie. a = ["v18.11, v16.7.0"]) with an array of strings (ie. b = ["v18.07", "v16.7.0"]) in versionSort(a, b).

minVersion(a) couldn't find the minimum version with an arrify string like a = ["v18.11, v16.7.0"].
That's why incorrect version history orders sometimes appeared.

Fixes: #45670

lpinca · 2022-12-03T20:08:35Z

See #45726 (comment).

welfoz · 2022-12-03T20:19:45Z

See #45726 (comment).

What does this mean? What should I do?

lpinca · 2022-12-03T22:06:04Z

It explains why the order is the one reported in #45670 which is not directly related to the changes done here. These changes might be ok but they do not fully fix the issue.

welfoz · 2022-12-04T00:04:48Z

It explains why the order is the one reported in #45670 which is not directly related to the changes done here. These changes might be ok but they do not fully fix the issue.

node/doc/api/util.md

Lines 1378 to 1392 in f6052c6

    
           <!-- YAML 
        
           added: 
        
             - v18.3.0 
        
             - v16.17.0 
        
           changes: 
        
             - version: v18.11.0 
        
               pr-url: https://github.com/nodejs/node/pull/44631 
        
               description: Add support for default values in input `config`. 
        
             - version: 
        
               - v18.7.0 
        
               - v16.17.0 
        
               pr-url: https://github.com/nodejs/node/pull/43459 
        
               description: add support for returning detailed parse information 
        
                            using `tokens` in input `config` and returned properties. 
        
           -->

In the example above, before sorting the version part of meta.changes looks like:

 ["v18.11.0", ["v18.7.0", "v16.17.0" ], [ "v18.3.0", "v16.17.0" ]]

With my commit implemented, the versionSort() function, after the minVersion(), will compare the same string for the last 2 indexes (

 "v16.17.0"

) and return 0. So, the initial order won't change.

https://developer.mozilla.org/enUS/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

Last point, in this case, when several similar minimal versions are in the "changes" part and in the added, deprecated or deleted part, the order will remain the same because they are the last ones pushed in the meta.changes array.

node/tools/doc/html.mjs

Lines 344 to 346 in f6052c6

    
           if (added.description) meta.changes.push(added); 
        
           if (deprecated.description) meta.changes.push(deprecated); 
        
           if (removed.description) meta.changes.push(removed);

When you build the doc, here is the new output:

I would be grateful if you could explain me what's missing 😄

lpinca · 2022-12-04T07:53:41Z

This is what happens on the main branch. The original issue refers to the latest v18.x version where the 'v16.17.0' is not present in the added array

node/doc/api/util.md

Lines 1025 to 1037 in 9ca57fa

    
           <!-- YAML 
        
           added: v18.3.0 
        
           changes: 
        
             - version: v18.11.0 
        
               pr-url: https://github.com/nodejs/node/pull/44631 
        
               description: Add support for default values in input `config`. 
        
             - version: 
        
               - v18.7.0 
        
               - v16.17.0 
        
               pr-url: https://github.com/nodejs/node/pull/43459 
        
               description: add support for returning detailed parse information 
        
                            using `tokens` in input `config` and returned properties. 
        
           -->

It seems that this commit db5aeed added the 'v16.17.0' version for the changes array but the author forgot to add it in the added array. The commit was then backported to the v18.x branch.

Of course we can manually add the 'v16.17.0' version in the added array of the v18.x-staging branch or wait until a new backported commit will fix the issue but I think we can go further and handle cases like this by moving this line

node/tools/doc/html.mjs

Line 344 in 81fa992

if (added.description) meta.changes.push(added);

after the meta.changes array is sorted

node/tools/doc/html.mjs

Line 348 in 81fa992

meta.changes.sort((a, b) => versionSort(a.version, b.version));

The added in version always comes first. There is no need to sort it.

welfoz · 2022-12-04T12:51:52Z

@lpinca You're right. I didn't see this that way.

I think there is no need to do the same for removed or deprecated versions as we need to move them at the beginning of the meta.changes array.
As they will always be newer versions they will automatically be sorted. If not, we could still push them directly at the beginning with meta.changes.unsift(removed)

tools/doc/html.mjs

lpinca · 2022-12-04T21:03:38Z

Test failures are related but I'm not sure why. This needs to be investigated.

welfoz · 2022-12-04T21:11:59Z

Test failures are related but I'm not sure why. This needs to be investigated.

I'm investigating in.
The first one is my fault, my commit message doesn't follow the rules of 72 characters / column max.

lpinca · 2022-12-04T21:30:07Z

diff --git a/test/doctool/test-doctool-html.mjs b/test/doctool/test-doctool-html.mjs
index 9323548221..6fbca13185 100644
--- a/test/doctool/test-doctool-html.mjs
+++ b/test/doctool/test-doctool-html.mjs
@@ -79,10 +79,11 @@ const testData = [
       '<div class="api_metadata">' +
       '<details class="changelog"><summary>History</summary>' +
       '<table><tbody><tr><th>Version</th><th>Changes</th></tr>' +
+      '<tr><td>v4.2.0</td><td><p>The <code>error</code> parameter can now be' +
+      'an arrow function.</p></td></tr>' +
       '<tr><td>v5.3.0, v4.2.0</td>' +
       '<td><p><span>Added in: v5.3.0, v4.2.0</span></p></td></tr>' +
-      '<tr><td>v4.2.0</td><td><p>The <code>error</code> parameter can now be' +
-      'an arrow function.</p></td></tr></tbody></table></details></div> ' +
+      '</tbody></table></details></div> ' +
       '<p>Describe <code>Foobar II</code> in more detail here.' +
       '<a href="http://man7.org/linux/man-pages/man1/fg.1.html"><code>fg(1)' +
       '</code></a></p></section><section>' +

This should fix the failing test.

welfoz · 2022-12-04T22:12:26Z

diff --git a/test/doctool/test-doctool-html.mjs b/test/doctool/test-doctool-html.mjs
index 9323548221..6fbca13185 100644
--- a/test/doctool/test-doctool-html.mjs
+++ b/test/doctool/test-doctool-html.mjs
@@ -79,10 +79,11 @@ const testData = [
       '<div class="api_metadata">' +
       '<details class="changelog"><summary>History</summary>' +
       '<table><tbody><tr><th>Version</th><th>Changes</th></tr>' +
+      '<tr><td>v4.2.0</td><td><p>The <code>error</code> parameter can now be' +
+      'an arrow function.</p></td></tr>' +
       '<tr><td>v5.3.0, v4.2.0</td>' +
       '<td><p><span>Added in: v5.3.0, v4.2.0</span></p></td></tr>' +
-      '<tr><td>v4.2.0</td><td><p>The <code>error</code> parameter can now be' +
-      'an arrow function.</p></td></tr></tbody></table></details></div> ' +
+      '</tbody></table></details></div> ' +
       '<p>Describe <code>Foobar II</code> in more detail here.' +
       '<a href="http://man7.org/linux/man-pages/man1/fg.1.html"><code>fg(1)' +
       '</code></a></p></section><section>' +

This should fix the failing test.

Yes! From what I understand, the test breaks because now we force the added version to be the last one.

welfoz · 2022-12-04T22:54:36Z

I needed to change the first commit message, so I did a rebase, and then a push --force.

@lpinca the test now passed in local!

welfoz · 2022-12-05T12:45:09Z

This command has 1 test failing because of an excessive duration:

Does anyone have any idea why this failed?

welfoz · 2022-12-11T20:26:38Z

@lpinca @panva do you know if it will be merged ?
Tell me if I need to do something else 😄

panva · 2022-12-11T20:30:06Z

@welfoz I only managed the labels while passing by, so, I don't know 🤷‍♂️.

lpinca · 2022-12-12T14:42:43Z

@welfoz can you please squash the commits?

welfoz · 2022-12-12T17:25:00Z

@welfoz can you please squash the commits?

Done! I've fetch the HEAD and squash all the commits :)

lpinca · 2022-12-12T17:26:57Z

Thanks, one last request, can you please replace "Therefore," in commit message with something like "Furthermore, there is". That "Therefore" seems misplaced.

welfoz · 2022-12-12T18:08:55Z

My bad, this is fixed!

welfoz · 2022-12-12T19:00:35Z

@lpinca I've changed the commit message again because the "Co-authored-by" part was misplaced.

lpinca · 2022-12-12T19:11:42Z

Now that you are at it, "Futhermore" -> "Furthermore" ;)

This fixes an error in parseYAML(text), the version sorting coudn't be right as we compared an arrify string (ie. a = ["v18.11, v16.7.0"]) with an array of strings (ie. b = ["v18.07", "v16.7.0"]) in versionSort(a, b). minVersion(a) couldn't find the minimum version with an arrify string like a = ["v18.11, v16.7.0"]. That's why incorrect version history orders sometimes appeared. Furthermore, no need to sort the added version as it always comes first. So, it can be the last one to be pushed in the meta.changes array. Fixes: nodejs#45670 Co-authored-by: Luigi Pinca <[email protected]>

welfoz · 2022-12-12T22:43:30Z

Now that you are at it, "Futhermore" -> "Furthermore" ;)

I'm now mastering the git rebase -i command haha

nodejs-github-bot · 2022-12-13T21:02:03Z

Commit Queue failed

- Loading data for nodejs/node/pull/45728
✔  Done loading data for nodejs/node/pull/45728
----------------------------------- PR info ------------------------------------
Title      tools: fix incorrect version history order (#45728)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     welfoz:fabien -> nodejs:main
Labels     doc, tools, review wanted
Commits    1
 - tools: fix incorrect version history order
Committers 1
 - Fabien MICHEL 
PR-URL: https://github.com/nodejs/node/pull/45728
Fixes: https://github.com/nodejs/node/issues/45670
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45728
Fixes: https://github.com/nodejs/node/issues/45670
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - tools: fix incorrect version history order
   ℹ  This PR was created on Sat, 03 Dec 2022 19:55:48 GMT
   ✔  Approvals: 1
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/45728#pullrequestreview-1203699118
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3689291523

nodejs-github-bot · 2022-12-13T21:13:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48456/

nodejs-github-bot · 2022-12-14T09:38:38Z

Landed in bfb5ba7

This fixes an error in parseYAML(text), the version sorting coudn't be right as we compared an arrify string (ie. a = ["v18.11, v16.7.0"]) with an array of strings (ie. b = ["v18.07", "v16.7.0"]) in versionSort(a, b). minVersion(a) couldn't find the minimum version with an arrify string like a = ["v18.11, v16.7.0"]. That's why incorrect version history orders sometimes appeared. Furthermore, no need to sort the added version as it always comes first. So, it can be the last one to be pushed in the meta.changes array. Fixes: #45670 Co-authored-by: Luigi Pinca <[email protected]> PR-URL: #45728 Reviewed-By: Luigi Pinca <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 3, 2022

welfoz marked this pull request as ready for review December 3, 2022 19:56

lpinca approved these changes Dec 4, 2022

View reviewed changes

tools/doc/html.mjs Outdated Show resolved Hide resolved

tools/doc/html.mjs Outdated Show resolved Hide resolved

tools/doc/html.mjs Outdated Show resolved Hide resolved

panva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 4, 2022

lpinca mentioned this pull request Dec 4, 2022

doc: fix Incorrect order in util.parseArgs history #45726

Closed

welfoz force-pushed the fabien branch from 80f67b1 to cc0f45a Compare December 4, 2022 22:50

panva added the review wanted PRs that need reviews. label Dec 11, 2022

welfoz force-pushed the fabien branch from cc0f45a to 80ff6f6 Compare December 12, 2022 17:22

welfoz force-pushed the fabien branch from 80ff6f6 to 29ae0d1 Compare December 12, 2022 18:07

welfoz force-pushed the fabien branch from 29ae0d1 to 971dcc3 Compare December 12, 2022 18:57

lpinca removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 12, 2022

welfoz force-pushed the fabien branch from 971dcc3 to 7f7ecaf Compare December 12, 2022 22:42

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 13, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 13, 2022

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 13, 2022

lpinca approved these changes Dec 13, 2022

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 13, 2022

lpinca added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 14, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 14, 2022

nodejs-github-bot merged commit bfb5ba7 into nodejs:main Dec 14, 2022

RafaelGSS mentioned this pull request Jan 2, 2023

v19.4.0 proposal #46061

Merged

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: fix incorrect version history order #45728

tools: fix incorrect version history order #45728

welfoz commented Dec 3, 2022

lpinca commented Dec 3, 2022

welfoz commented Dec 3, 2022

lpinca commented Dec 3, 2022 •

edited

Loading

welfoz commented Dec 4, 2022 •

edited

Loading

lpinca commented Dec 4, 2022 •

edited

Loading

welfoz commented Dec 4, 2022

lpinca commented Dec 4, 2022

welfoz commented Dec 4, 2022

lpinca commented Dec 4, 2022 •

edited

Loading

welfoz commented Dec 4, 2022

welfoz commented Dec 4, 2022

welfoz commented Dec 5, 2022

welfoz commented Dec 11, 2022

panva commented Dec 11, 2022

lpinca commented Dec 12, 2022

welfoz commented Dec 12, 2022

lpinca commented Dec 12, 2022

welfoz commented Dec 12, 2022

welfoz commented Dec 12, 2022

lpinca commented Dec 12, 2022 •

edited

Loading

welfoz commented Dec 12, 2022

nodejs-github-bot commented Dec 13, 2022

nodejs-github-bot commented Dec 13, 2022

nodejs-github-bot commented Dec 14, 2022

tools: fix incorrect version history order #45728

tools: fix incorrect version history order #45728

Conversation

welfoz commented Dec 3, 2022

lpinca commented Dec 3, 2022

welfoz commented Dec 3, 2022

lpinca commented Dec 3, 2022 • edited Loading

welfoz commented Dec 4, 2022 • edited Loading

lpinca commented Dec 4, 2022 • edited Loading

welfoz commented Dec 4, 2022

lpinca commented Dec 4, 2022

welfoz commented Dec 4, 2022

lpinca commented Dec 4, 2022 • edited Loading

welfoz commented Dec 4, 2022

welfoz commented Dec 4, 2022

welfoz commented Dec 5, 2022

welfoz commented Dec 11, 2022

panva commented Dec 11, 2022

lpinca commented Dec 12, 2022

welfoz commented Dec 12, 2022

lpinca commented Dec 12, 2022

welfoz commented Dec 12, 2022

welfoz commented Dec 12, 2022

lpinca commented Dec 12, 2022 • edited Loading

welfoz commented Dec 12, 2022

nodejs-github-bot commented Dec 13, 2022

nodejs-github-bot commented Dec 13, 2022

nodejs-github-bot commented Dec 14, 2022

lpinca commented Dec 3, 2022 •

edited

Loading

welfoz commented Dec 4, 2022 •

edited

Loading

lpinca commented Dec 4, 2022 •

edited

Loading

lpinca commented Dec 4, 2022 •

edited

Loading

lpinca commented Dec 12, 2022 •

edited

Loading