Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Topics subtopics are not respected without a name #48

Closed
amphro opened this issue Aug 30, 2018 · 7 comments
Closed

Topics subtopics are not respected without a name #48

amphro opened this issue Aug 30, 2018 · 7 comments

Comments

@amphro
Copy link
Contributor

amphro commented Aug 30, 2018

Repo:
Given the following topics list in the oclif package.json:

"topics": {
      "t1": {
        "description": "desc for t1",
        "subtopics": {
          "t1-1": {
            "description": "desc for t1-1",
            "subtopics": {
              "t1-1-1": {
                "description": "desc for t1-1-1"
              },
              "t1-1-2": {
                "description": "desc for t1-1-2"
              }
            }
          }
        }
      }
    }
  },

Then run ./bin/run t1 --help.

Expected results:


USAGE
  $ bin t1:COMMAND

COMMANDS
  t1:t1-1  desc for t1-1

Actual results:


USAGE
  $ bin t1:COMMAND

COMMANDS
  t1:t1-1  desc for the first command in t1-1

Potential Fix
I debugged this a little big and narrowed it down to this line: https://github.com/oclif/config/blob/master/src/plugin.ts#L249

Specifically ${base}${input[k].name}. It seems like what you want here is actually ``${base}${k}` since you are still dealing with the object, but not sure the original intent. The code looks like it wants a name property in the subtopic property but that seems counter-intuitive since I'm using a map. If I make the change locally, everything works fine, but I'm not sure if I am breaking another case.

If I have time, I'll try to create a PR.

@jdx
Copy link
Contributor

jdx commented Aug 30, 2018

yeah this is a little odd. For a little background that explains why this is like this: I went back and forth about whether or not there should be a 'name' field. I never wanted the name field in the configuration, my intention was for that to be populated inside the code with the key of the object it was inside of. It makes it a little easier to pass around a complete topic object that has the name attached. See in places like this: https://github.com/oclif/plugin-help/blob/master/src/index.ts#L48

So I think we do need the name. I'm not sure where it's populating that field exactly though, I would expect it to be in this repo but I can't seem to find it.

@amphro
Copy link
Contributor Author

amphro commented Aug 30, 2018

Also worth noting, I can't get other variations to work. Like

    "topics": [{
      "name": "t1",
      "description": "desc for t1",
      "subtopics": [{
        "name": "t1-1",
        "description": "desc for t1-1",
        "subtopics": [{
          "name": "t1-1-1",
          "description": "desc for t1-1-1"
        }, {
          "name": "t1-1-2",
          "description": "desc for t1-1-2"
        }]
      }]
    }]

Do you have a working example for nested topics?

@jdx
Copy link
Contributor

jdx commented Aug 30, 2018

You're mixing up how we reference topics in the code vs how they should be referenced in the config, they're not the same. In the code, we deal with a single array of all the topics including all subtopics but in the config it's a nested object. It's strange, but I've found this is the easiest way to deal with them in both places.

@jdx
Copy link
Contributor

jdx commented Aug 30, 2018

Having said that, we don't do much work with deeply nested things so I wouldn't be surprised if there was a bug inside of this logic. Let me see if I can work up an example.

@amphro
Copy link
Contributor Author

amphro commented Aug 30, 2018

Alright, you are right with adding names. This works:

    "topics": {
      "t1": {
        "name": "t1",
        "description": "desc for t1",
        "subtopics": {
          "t1-1": {
            "name": "t1-1",
            "description": "desc for t1-1",
            "subtopics": {
              "t1-1-1": {
                "name": "t1-1-1",
                "description": "desc for t1-1-1"
              },
              "t1-1-2": {
                "name": "t1-1-2",
                "description": "desc for t1-1-2"
              }
            }
          }
        }
      }
    }

Still seems odd to me that I need to duplicate the name when the framework can easily fo that for me. Also, I'm still not sure why my previous example didn't work, because it has a name and you have code in there to handle arrays.

@jdx
Copy link
Contributor

jdx commented Aug 30, 2018

This is incorrect behavior as to how it should work, I was just explaining why it's like this. How you imagined it is how it should work. I'm going to take a closer look.

jdx added a commit that referenced this issue Aug 30, 2018
jdx added a commit that referenced this issue Aug 30, 2018
@jdx jdx assigned amphro and unassigned amphro Aug 30, 2018
@jdx jdx closed this as completed in #49 Aug 30, 2018
jdx added a commit that referenced this issue Aug 30, 2018
jdx pushed a commit that referenced this issue Aug 30, 2018
<a name="1.7.3"></a>
## [1.7.3](v1.7.2...v1.7.3) (2018-08-30)

### Bug Fixes

* default subtopic names ([#49](#49)) ([c084b64](c084b64)), closes [#48](#48)
@oclif-bot
Copy link
Contributor

🎉 This issue has been resolved in version 1.7.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants