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

fix: help command #593

Closed
wants to merge 10 commits into from
Closed

fix: help command #593

wants to merge 10 commits into from

Conversation

sambhavgupta0705
Copy link
Member

Description

  • ...
  • ...
  • ...

Related issue(s)

#559

Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should create a new folder called hooks, instead of adding this in the errors folder

package.json Outdated
@@ -105,10 +104,13 @@
"main": "lib/index.js",
"oclif": {
"commands": "./lib/commands",
"hooks": {
"command_not_found": [
"./lib/hooks/command_not_found/example"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"./lib/hooks/command_not_found/example"
"./lib/src/errors/command-not-found"

I think this should be the path of the hook, not really sure, looking at this example it looks like that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Souvikns i've added the hook in hooks/command-not-found folder, it should work right ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be a file path, check that after building the project does that path has any file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's just file paths that we have to link back in package.json. It's working build.

I just separated the hooks into folders based on events for further clarification if anyone wants to add new hooks.

@kaushik-rishi
Copy link
Contributor

@Souvikns Just a quick question before i bail out, how are we supposed to prevent the
@oclif/plugin-not-found from triggering, inside the hook that we are working with ?

@Souvikns
Copy link
Member

Souvikns commented May 25, 2023

The plugin we are using right now is actually a hook, you can use that as an example https://github.com/oclif/plugin-not-found

@kaushik-rishi
Copy link
Contributor

The plugin we are using right now is actually a hook, you can use that as an example https://github.com/oclif/plugin-not-found

But we would still need the oclif/plugin-not-found for any other user entered commands which donot exist right ?

We basically need this new hook on top of oclif/plugin-not-found as far as i get it. please correct me.

@sambhavgupta0705
Copy link
Member Author

sambhavgupta0705 commented May 25, 2023

image
This is the output @Souvikns

@sambhavgupta0705
Copy link
Member Author

The plugin we are using right now is actually a hook, you can use that as an example https://github.com/oclif/plugin-not-found

yes I read the blog and the hook was setup inside the plugin

@Souvikns
Copy link
Member

I don't think we need the old plugin we are replacing it with our custom hook

@kaushik-rishi
Copy link
Contributor

kaushik-rishi commented May 25, 2023

image

This is the output @Souvikns

I just logged the options passed for reference. You can build on top of this.

@sambhavgupta0705
Copy link
Member Author

@kaushik-rishi I think we should not console.log this whole message and just console help command doesn't exist try --help

@Souvikns
Copy link
Member

That output is comping from logging the opts right, now you just have to have a custom warning about it. I think if you see that log that means that the hook is connected to the CLI

@kaushik-rishi
Copy link
Contributor

@sambhavgupta0705 yes, i just logged it for your reference.
i've written an if condition below it inside of which you can build.

@kaushik-rishi
Copy link
Contributor

@sambhavgupta0705 please remove the errors folder. just reminding.

@sambhavgupta0705
Copy link
Member Author

sambhavgupta0705 commented May 25, 2023

@sambhavgupta0705 please remove the errors folder. just reminding.

okay the one made by me

@sambhavgupta0705
Copy link
Member Author

@Souvikns the PR is ready for review

@sambhavgupta0705 sambhavgupta0705 marked this pull request as ready for review May 25, 2023 14:57
@kaushik-rishi
Copy link
Contributor

package-lock.json shouldn't technically change.

As we didn't install or uninstall any package I think we can safely revert package-lock.json changes.

@kaushik-rishi
Copy link
Contributor

I don't think we need the old plugin we are replacing it with our custom hook

@Souvikns need your suggestions 🙂

i understand, but what about the fallback for other commands, the hook developed here is handling just the help command and suggesting the user to use --help.
What if i use something like asyncapi generte (intentional typo), we would still need the plugin-not-found to give us a fallback option to suggestions from the plugin right ?

image


Also, @sambhavgupta0705 can we rather find a way to just bail out and exit after the console.log in the hook, not to forward the entire case to plugin-not-found module ?

image

@sambhavgupta0705
Copy link
Member Author

Yes we can add .exit functionality here.

@kaushik-rishi
Copy link
Contributor

kaushik-rishi commented May 25, 2023

@sambhavgupta0705 for your reference.
follow the last line of this documentation https://oclif.io/docs/hooks.
you can use this.exit() to exit out instead of going further.

Try it once, you might see other issues. Just a hint forward. 🙂

let @Souvikns confirm if this is what we are looking forward to do.

if (opts.id === 'help') {
console.log('help command doesn\'t exist try --help');
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.exit() is what we would want here (incase we want to exit out)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will implement it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work, I mean if the control ever goes to the plugin it will say to use asyncapi help command and that does not exist.

@sambhavgupta0705
Copy link
Member Author

@kaushik-rishi I tried adding this.exit() but the plugin console was always get printed

@kaushik-rishi
Copy link
Contributor

@kaushik-rishi I tried adding this.exit() but the plugin console was always get printed

It is probably due to asynchronous nauigure, i'm afraid i don't have a solution for this right away.

I can explain the situation better.
May be it's like once the hook is triggered both the plugins (the one we wrote and the plugin-not-found) and being called at the same time. It is exiting fine but before exiting it's printing the first line of the plugin-not-founds prompt.

@sambhavgupta0705
Copy link
Member Author

Yess

@kaushik-rishi
Copy link
Contributor

Yess

I have a couple of ideas, Let @Souvikns catch up and review the discussion uptill now. 🙂

We can easily make our own hook for handling every command that isn't found. Let's carry it further.

@Souvikns
Copy link
Member

Yeah, the plugin used to suggest or at least try to suggest an alternative command if one is wrong, I think we can build this feature, you can take inspiration from the plugin if you want.

@kaushik-rishi
Copy link
Contributor

Yeah, the plugin used to suggest or at least try to suggest an alternative command if one is wrong, I think we can build this feature, you can take inspiration from the plugin if you want.

@Souvikns
I'm not very aware of the licenes, but looking at this (licence), are we allowed to copy the code into our hook and then modify as per our requirements ?

Copy link
Contributor

@kaushik-rishi kaushik-rishi May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sambhavgupta0705
please revert the package-lock.json changes.

@Souvikns
Copy link
Member

You write the code, and you can take inspiration from it, and write it in your own way, I don't know about the license, @derberg are we breaking any rules here?

@kaushik-rishi
Copy link
Contributor

kaushik-rishi commented May 26, 2023

You write the code, and you can take inspiration from it, and write it in your own way, I don't know about the license, @derberg are we breaking any rules here?

If we have the ability to re-use the code, we can skip rewriting it 🙂 . Let @derberg clarify. We can write our own handler in the worst case, Won't be that difficult.

@Souvikns
Copy link
Member

Souvikns commented Jun 6, 2023

@sambhavgupta0705 @kaushik-rishi, @derberg is not available right now, you can go ahead and start building the suggestion feature, no need to reuse but if you want you can take inspiration from the existing library.

@kaushik-rishi
Copy link
Contributor

Sure @Souvikns

@sambhavgupta0705
Copy link
Member Author

@sambhavgupta0705 @kaushik-rishi, @derberg is not available right now, you can go ahead and start building the suggestion feature, no need to reuse but if you want you can take inspiration from the existing library.

Yup just busy with some work,will start working on this by this week.

@sambhavgupta0705
Copy link
Member Author

hey @kaushik-rishi ,what other changes do we need to do?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.0% 1.0% Duplication

@kaushik-rishi
Copy link
Contributor

hey @kaushik-rishi ,what other changes do we need to do?

Hey @sambhavgupta0705
Was a bit busy since a couple of days.
We can take inspiration from the @oclif/not-found and recreate some workflow to let the user know that they've entered a command that is not yet registered.

@derberg
Copy link
Member

derberg commented Jun 27, 2023

oclif is done under MIT license, so it is definitely fine to copy some code. Just attribute original creators in the readme

@Souvikns
Copy link
Member

@sambhavgupta0705 let us know if anything is blocking you or if you need any help.

@sambhavgupta0705
Copy link
Member Author

@Souvikns just got busy with mentorship program work .I will soon find a way to resolve this ASAP

@Souvikns
Copy link
Member

@sambhavgupta0705 any updates on this?

@sambhavgupta0705
Copy link
Member Author

ohh sorry I totally forgot about this

@Souvikns
Copy link
Member

@sambhavgupta0705 can you resolve the conflict

Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

4 New issues
0 Security Hotspots
No data about Coverage
1.0% Duplication on New Code

See analysis details on SonarCloud

@peter-rr
Copy link
Member

peter-rr commented Mar 21, 2024

@Souvikns @derberg
Should we close this PR as the bug related is already solved by #843 ?

@derberg
Copy link
Member

derberg commented Apr 10, 2024

closing as looks like not needed

@derberg derberg closed this Apr 10, 2024
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.

5 participants