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

feat: support for custom commands #8

Merged
merged 8 commits into from Jul 30, 2022

Conversation

pocketken
Copy link
Contributor

As mentioned in #7, it'd be nice to extend the shell parser to be able to handle additional commands as though they were built-ins/in the system path. Here's a quick take at implementing support by means of extending the CommandBuilder with a handle() method for registering a custom command handler/callback. This should fit in nicely with the custom builder pattern outlined in the documentation.

PR is not so much WIP as at a point where I could use some feedback:

  • does this fit with the direction you want things to go? I tried to be somewhat "non-invasive" in terms of how I implemented this, but as its a new project you may have already had ideas on how you wanted to implement this sort of thing down the road.
  • not sure I like handle, but command() is obviously taken and customCommand() didn't feel right either.
  • if you've got a few commands, one at a time would be tedious. Maybe just take a Record<string, CustomCommandHandler>?

Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Yeah, I like this and good idea! I think we could make a couple changes though.

src/command.ts Outdated Show resolved Hide resolved
mod.test.ts Outdated Show resolved Hide resolved
mod.test.ts Outdated Show resolved Hide resolved
@pocketken
Copy link
Contributor Author

@dsherret hmm; I am getting different behavior when I run this locally in Windows. Both the original and modified version pass here, which now that I think about it noThrow() would in fact, not throw...

I need another coffee.

@dsherret
Copy link
Owner

dsherret commented Jul 30, 2022

It's probably finding a cd command on mac (via resolveCommand)

@pocketken
Copy link
Contributor Author

It's probably finding a cd command on mac (via resolveCommand)

Of course it is! ..fires up mac..

$ which cd
/usr/bin/cd
$ which echo
/bin/echo
$ which export
$

export wins! I'll update.

Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

This all looks very good to me. Thanks a lot! I'm just going to make a few small changes to it myself then merge the PR and do a release.

@pocketken
Copy link
Contributor Author

👍 no problem, glad to contribute back where I can.

Thanks again for throwing this together!


const result = await commandBuilder
// now includes the 'true' command
.command("true && echo yay")
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to build in true and false by the way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I knew you were going to say that :)

I have a few more tasks yet to work through, so, I imagine there will be a PR or two coming with more commands. I'll put those on my list :)

@dsherret dsherret merged commit 65080fc into dsherret:main Jul 30, 2022
@dsherret
Copy link
Owner

Thanks again for this and all your other PRs! I released version 0.8.0.

@pocketken pocketken deleted the feat-custom-commands branch July 30, 2022 21:30
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