Refactor `getCommandAndParents()` into `Command._getCommandAndAncestors()` and use consistently by aweebit · Pull Request #1939 · tj/commander.js

aweebit · 2023-08-05T09:09:42Z

Cherry-picked from #1938.

A purely cosmetic PR Introducing no functional changes or changes visible to the user.

Replaces getCommandAndParents() with Command._getCommandAndAncestors()

…because an instance method is where the function logically belongs.
Renamed because "ancestors" is more precise than "parents".

Also changes the code to make use of the function whenever appropriate. (I thought it was a good idea after introducing currentParent replacing parent in #1938. I ended up getting rid of it and simply upgrading the parent semantics, but who knows what changes to the parent resolution could be made in the future. It is better to keep the code for getting the ancestors in one place.)

Replaces getCommandAndParents(): - turned into an instance method - used "ancestors" instead of "parents" because it is more precise (cherry picked from commit aa280af)

(cherry picked from commit 777a452)

aweebit · 2023-08-05T09:17:08Z

(Force push is due to a reset in order to change the commit message borrowed from aa280af so that it does not say anything about the parents array only introduced in #1938.)

shadowspawn · 2023-08-09T06:13:38Z

This comment is just musing. No action items.

I had considered these when I wrote the code, but was 50/50 on a couple, and willing to change.

…because an instance method is where the function logically belongs.

Ok.

Renamed because "ancestors" is more precise than "parents".

My concern was that ancestors might be more correct but less understandable to some readers. However, ancestor is certainly tree terminology. Ok.

https://en.wikipedia.org/wiki/Tree_(data_structure)
Ancestor
A node reachable by repeated proceeding from child to parent.

shadowspawn · 2023-08-09T06:30:38Z

Also changes the code to make use of the function whenever appropriate. (...but who knows what changes to the parent resolution could be made in the future. It is better to keep the code for getting the ancestors in one place.)

No, or at least not as strong as you describe.

I did wonder about making the navigation more like an internal detail (so use the helper routine everywhere), but decided against that and have not changed my mind. The subcommand tree is navigable downwards using the .commands() links and up by using the .parent links. These are public properties and will be used in external code. It is fine to use them in code.

So using the helper routine and array methods to replace manual loops is ok when it is an improvement or at least directly equivalent. But it is not appropriate when it requires extra steps or is awkward.

shadowspawn · 2023-08-09T06:36:32Z

lib/command.js

      let candidateFlags = [];
-      let command = this;
-      do {
+      for (const command of this._getCommandAndAncestors()) {


Can this be a forEach? (I don't like the for ( of ).

Could be if not for the early exit that is impossible with forEach. I guess this is one of the cases where iterating manually via .parent is more appropriate.

shadowspawn · 2023-08-09T06:38:22Z

lib/help.js

    const globalOptions = [];
-    for (let parentCmd = cmd.parent; parentCmd; parentCmd = parentCmd.parent) {
-      const visibleOptions = parentCmd.options.filter((option) => !option.hidden);
+    cmd._getCommandAndAncestors().slice(1).forEach((ancestorCmd) => {


No. Having to use the slice makes this too subtle for my liking.

(Ok with the active variable changing to ancestorCmd though.)

shadowspawn · 2023-08-09T06:39:42Z

lib/help.js

-    }
-    return parentCmdNames + cmdName + ' ' + cmd.usage();
+    let ancestorCmdNames = '';
+    cmd._getCommandAndAncestors().slice(1).forEach((ancestorCmd) => {


No. Having to use the slice makes this too subtle for my liking.
Ok with the active variables changing to ancestorX though.

shadowspawn · 2023-08-09T06:42:48Z

So using the helper routine and array methods to replace manual loops is ok when it is an improvement or at least directly equivalent. But it is not appropriate when it requires extra steps or is awkward.

This only ended up affecting a couple of the changes! I had seen them on first scan and feared there might be more, so wanted to explain my reasoning. Added comments on the lines of concern.

shadowspawn

Was dubious about this but after working through my concerns, yup, ok!

Changes requested on three of the lines.

Only call when all elements are to be iterated. Resort to manual iteration via .parent in other cases.

shadowspawn

LGTM

abetomo

+1

aweebit added 2 commits August 5, 2023 12:13

Introduce _getCommandAndAncestors()

2005522

Replaces getCommandAndParents(): - turned into an instance method - used "ancestors" instead of "parents" because it is more precise (cherry picked from commit aa280af)

Use _getCommandAndAncestors() consistently

da5a499

(cherry picked from commit 777a452)

aweebit force-pushed the feature/_getCommandAndAncestors branch from 01251dc to da5a499 Compare August 5, 2023 09:14

shadowspawn added the on hold label Aug 8, 2023

shadowspawn removed the on hold label Aug 9, 2023

shadowspawn reviewed Aug 9, 2023

View reviewed changes

shadowspawn requested changes Aug 9, 2023

View reviewed changes

Use _getCommandAndAncestors() less aggressively

8b11122

Only call when all elements are to be iterated. Resort to manual iteration via .parent in other cases.

aweebit requested a review from shadowspawn August 10, 2023 01:06

shadowspawn approved these changes Aug 10, 2023

View reviewed changes

aweebit mentioned this pull request Aug 13, 2023

Pull request catalog by @aweebit #1964

Closed

abetomo approved these changes Aug 19, 2023

View reviewed changes

abetomo merged commit f76a734 into tj:develop Aug 19, 2023

shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Aug 22, 2023

shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Feb 3, 2024

Uh oh!

Conversation

aweebit commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweebit commented Aug 5, 2023

Uh oh!

shadowspawn commented Aug 9, 2023

Uh oh!

shadowspawn commented Aug 9, 2023

Uh oh!

shadowspawn Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

aweebit Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

shadowspawn Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

shadowspawn Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

shadowspawn Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadowspawn Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadowspawn commented Aug 9, 2023

Uh oh!

shadowspawn left a comment

Choose a reason for hiding this comment

Uh oh!

shadowspawn left a comment

Choose a reason for hiding this comment

Uh oh!

abetomo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aweebit commented Aug 5, 2023 •

edited

Loading

shadowspawn Aug 9, 2023 •

edited

Loading

shadowspawn Aug 9, 2023 •

edited

Loading