Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test for #755 #756

Closed
wants to merge 2 commits into from
Closed

Test for #755 #756

wants to merge 2 commits into from

Conversation

KishiTheMechanic
Copy link

@KishiTheMechanic KishiTheMechanic commented Sep 17, 2021

Related this issue #627 , #755
Modified tests also.

@dorner
Copy link

dorner commented Sep 20, 2021

@KishiTheMechanic this doesn't seem to be fixing the behavior you mentioned earlier. This seems to remove the thor part of the help output but doesn't fix it for doubly nested subcommands. Unless there's another missing test somewhere?

@KishiTheMechanic
Copy link
Author

I think it's ok solution for now because if there's no thor part, we can see them as after the command of which you entered with help.

@dorner
Copy link

dorner commented Sep 20, 2021

@KishiTheMechanic can you add a test demonstrating this behavior please?

@KishiTheMechanic
Copy link
Author

What do you mean?
I modified the tests related this changing?

@dorner
Copy link

dorner commented Sep 20, 2021

The tests are all showing the main case of removing "thor" but they are not showing any new case that fixes the problem originally stated (where nested subcommands don't show the parent command).

@KishiTheMechanic
Copy link
Author

I thought removing "thor" is the solution for the state (where nested subcommands don't show the parent command).
Because the state's problem is confusing us about nested subcommands because it shows the head but not middle.
If it shows only after part of which command user inputed, we can know it is the help for the sub commands.
Doesn't it work?

@dorner
Copy link

dorner commented Sep 20, 2021

This is why I'd like to see a test demonstrating the issue so we can confirm that the fix works for the original case. 😄

@KishiTheMechanic
Copy link
Author

for example,

➜  souls-doc git:(main) souls api generate help 
Commands:
  souls generate connection [CLASS_NAME]                                 # Generate GraphQL Connection from schema.rb
  souls generate delete_all  [CLASS_NAME]                                # Generate Scaffold All Tables from schema.rb
  souls generate edge [CLASS_NAME]                                       # Generate GraphQL Edge from schema.rb
  souls generate help [COMMAND]                                          # Describe subcommands or one specific subcommand
  souls generate job [CLASS_NAME]                                        # Generate Job File in Worker
  souls generate mailer [MAILER_NAME]                                    # Generate Mailer Template in Worker
  souls generate manager [MANAGER_NAME] --mutation, --mutation=MUTATION  # Generate GraphQL Mutation Template
  souls generate model [CLASS_NAME]                                      # Generate Model Template
  souls generate mutation [CLASS_NAME]                                   # Generate GraphQL Mutation from schema.rb
  souls generate policy [CLASS_NAME]                                     # Generate Policy File Template
  souls generate query [CLASS_NAME]                                      # Generate GraphQL Query from schema.rb
  souls generate resolver [CLASS_NAME]                                   # Generate GraphQL Resolver from schema.rb
  souls generate rspec_factory [CLASS_NAME]                              # Generate Rspec Factory Test from schema.rb
  souls generate rspec_model [CLASS_NAME]                                # Generate Rspec Model Test from schema.rb
  souls generate rspec_mutation [CLASS_NAME]                             # Generate Rspec Mutation Test from schema.rb
  souls generate rspec_policy [CLASS_NAME]                               # Generate Rspec Policy Test from schema.rb
  souls generate rspec_query [CLASS_NAME]                                # Generate Rspec Query Test from schema.rb
  souls generate rspec_resolver [CLASS_NAME]                             # Generate Rspec Resolver Test from schema.rb
  souls generate scaffold [CLASS_NAME]                                   # Generate Scaffold from schema.rb
  souls generate scaffold_all                                            # Generate Scaffold All Tables from schema.rb
  souls generate type [CLASS_NAME]                                       # Generate GraphQL Type from schema.rb

➜  souls-doc git:(main) souls api help
Commands:
  souls api generate [COMMAND]  # souls api generate Commands
  souls api help [COMMAND]      # Describe subcommands or one specific subcommand
  souls api update [COMMAND]    # souls api update Commands

This is the problem.

Then I changed to below.

➜  souls-doc git:(main) souls api generate help 
Commands:
  generate connection [CLASS_NAME]                                 # Generate GraphQL Connection from schema.rb
  generate delete_all  [CLASS_NAME]                                # Generate Scaffold All Tables from schema.rb
  generate edge [CLASS_NAME]                                       # Generate GraphQL Edge from schema.rb
  generate help [COMMAND]                                          # Describe subcommands or one specific subcommand
  generate job [CLASS_NAME]                                        # Generate Job File in Worker
  generate mailer [MAILER_NAME]                                    # Generate Mailer Template in Worker
  generate manager [MANAGER_NAME] --mutation, --mutation=MUTATION  # Generate GraphQL Mutation Template
  generate model [CLASS_NAME]                                      # Generate Model Template
  generate mutation [CLASS_NAME]                                   # Generate GraphQL Mutation from schema.rb
  generate policy [CLASS_NAME]                                     # Generate Policy File Template
  generate query [CLASS_NAME]                                      # Generate GraphQL Query from schema.rb
  generate resolver [CLASS_NAME]                                   # Generate GraphQL Resolver from schema.rb
  generate rspec_factory [CLASS_NAME]                              # Generate Rspec Factory Test from schema.rb
  generate rspec_model [CLASS_NAME]                                # Generate Rspec Model Test from schema.rb
  generate rspec_mutation [CLASS_NAME]                             # Generate Rspec Mutation Test from schema.rb
  generate rspec_policy [CLASS_NAME]                               # Generate Rspec Policy Test from schema.rb
  generate rspec_query [CLASS_NAME]                                # Generate Rspec Query Test from schema.rb
  generate rspec_resolver [CLASS_NAME]                             # Generate Rspec Resolver Test from schema.rb
  generate scaffold [CLASS_NAME]                                   # Generate Scaffold from schema.rb
  generate scaffold_all                                            # Generate Scaffold All Tables from schema.rb
  generate type [CLASS_NAME]                                       # Generate GraphQL Type from schema.rb

➜  souls-doc git:(main) souls api help
Commands:
  api generate [COMMAND]  # souls api generate Commands
  api help [COMMAND]      # Describe subcommands or one specific subcommand
  api update [COMMAND]    # souls api update Commands

@dorner
Copy link

dorner commented Sep 20, 2021

Ah OK - so rather than adding in the api piece, you're removing the souls piece. I don't think this is the direction I'd like to go - I'd prefer adding in the full path for the best understandability. This fix is a bit better than the current implementation but it's not what I'd want to see.

@KishiTheMechanic
Copy link
Author

Yes, it's a small progress but actually better than now I think.

@dorner
Copy link

dorner commented Sep 20, 2021

Yes... but if we're going to go ahead and try to fix this then I'd rather fix it the right way rather than doing a half-fix now and a full fix later.

@KishiTheMechanic
Copy link
Author

Sounds great!

@KishiTheMechanic
Copy link
Author

Could you please let me upload the folk version to the gem (like thor_temp) as temporary solution for now?
I think it takes for a while to full fix it so.
I will delete the gem after this issue closed.

@dorner
Copy link

dorner commented Sep 20, 2021

@KishiTheMechanic not sure I understand the question. You're free to use your own fork in your projects if you like, just reference your Github repo:

gem thor, git: https://github.com/KishiTheMechanic/thor

@KishiTheMechanic
Copy link
Author

KishiTheMechanic commented Sep 20, 2021

Yes, but I can't use it on our gem...
We need to specify it as dependency to publish our gem (SOULs) so.
https://rubygems.org/gems/souls
By the way, thank you always so much to contribute it.

@dorner
Copy link

dorner commented Sep 20, 2021

Yeah... unfortunately there's no good way to specify a fork as a dependency on a gem. However, you can make a note in your README that to use it you need to add the fork of the thor gem to your Gemfile as specified. You can also add it to your gem's Gemfile (as opposed to its gemspec) so your tests pass.

Having said that, the best solution would indeed be to put in the "real" fix so we can merge 😄

@KishiTheMechanic
Copy link
Author

After all, I think you should accept this temporary solution now.
Because it seems very difficult to get nested commands in this architecture (If you know, please tell me).
And the bug has taken almost 2 years to fix which means also the problem is hard.

All Thor made CLI keeps lying until now. I think it's huge problem.

After this solution, at least the CLIs stop lying.

@KishiTheMechanic
Copy link
Author

I think we shouldn't nest commands too much in Thor and will refactor our gem to make shallow structure because it's not the only problem in too much nested commands.
Thank you.

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

Successfully merging this pull request may close these issues.

None yet

2 participants