tools: fix incorrect version history order #45728
Conversation
nodejs-github-bot
added
doc
|
See #45726 (comment). |
What does this mean? What should I do? |
|
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. |
Lines 1378 to 1392 in f6052c6 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.
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 Lines 344 to 346 in f6052c6
I would be grateful if you could explain me what's missing 😄 |
|
This is what happens on the Lines 1025 to 1037 in 9ca57fa It seems that this commit db5aeed added the Of course we can manually add the Line 344 in 81fa992 after the Line 348 in 81fa992 The added in version always comes first. There is no need to sort it. |
|
@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 |
panva
added
the
commit-queue-squash
|
Test failures are related but I'm not sure why. This needs to be investigated. |
I'm investigating in. |
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.
|
|
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! |
|
Does anyone have any idea why this failed? |
|
@welfoz I only managed the labels while passing by, so, I don't know 🤷♂️. |
|
@welfoz can you please squash the commits? |
Done! I've fetch the HEAD and squash all the commits :) |
|
Thanks, one last request, can you please replace "Therefore," in commit message with something like "Furthermore, there is". That "Therefore" seems misplaced. |
|
My bad, this is fixed! |
|
@lpinca I've changed the commit message again because the "Co-authored-by" part was misplaced. |
|
Now that you are at it, "Futhermore" -> "Furthermore" ;) |
lpinca
removed
the
commit-queue-squash
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 <luigipinca@gmail.com>
I'm now mastering the git rebase -i command haha |
lpinca
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
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/.ncuhttps://github.com/nodejs/node/actions/runs/3689291523 |
github-actions
bot
removed
the
request-ci
lpinca
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
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 <luigipinca@gmail.com> PR-URL: #45728 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <luigipinca@gmail.com> PR-URL: #45728 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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