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

RegisterCommands method #706

Merged
merged 8 commits into from
Dec 30, 2022
Merged

RegisterCommands method #706

merged 8 commits into from
Dec 30, 2022

Conversation

tkow
Copy link
Contributor

@tkow tkow commented Dec 24, 2022

Close #705.

I also add two sections named with Command Runner and Regsitere Commands to your docs and fix links broken I found in 4a3dd2d.

apps/docs/pages/commander.md

スクリーンショット 2022-12-25 14 57 27

apps/docs/pages/api.md

スクリーンショット 2022-12-25 14 57 27

@nx-cloud
Copy link

nx-cloud bot commented Dec 24, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dfdf7cd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@changeset-bot
Copy link

changeset-bot bot commented Dec 24, 2022

🦋 Changeset detected

Latest commit: dfdf7cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
nest-commander Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tkow
Copy link
Contributor Author

tkow commented Dec 24, 2022

@jmcdo29
Please, close it if you don't like it as I describing in #705.

@tkow tkow changed the title RegisterProviders method RegisterCommands method Dec 24, 2022
export abstract class CommandRunner {
static registerCommands(meta: string = CommandMeta): CommandRunnerClass[] {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we call this registerWithSubCommands? That way the intent is clear. The code is actually really clean, and if the command is clear enough then I don't see why we can't add it in. Thanks for adding the tests too, showing it works is really nice.

The only other thing I would ask is can you update the documentation? Just a new section in the Commander section would do

@jmcdo29
Copy link
Owner

jmcdo29 commented Dec 25, 2022

Oh, also, would you mind running pnpm changeset and following the wizard to create a minor changeset for the nest-commander package? That will help automate the release of this once I merge it in.

@tkow tkow force-pushed the feat/command-register branch 2 times, most recently from 3b17d2c to 6e260dd Compare December 25, 2022 05:51
Copy link
Owner

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

A couple of grammatical revisions. Thanks for all the work!

apps/docs/src/pages/en/api.md Outdated Show resolved Hide resolved
apps/docs/src/pages/en/features/commander.md Outdated Show resolved Hide resolved
apps/docs/src/pages/en/features/commander.md Outdated Show resolved Hide resolved
@jmcdo29
Copy link
Owner

jmcdo29 commented Dec 25, 2022

If everything (save for the sending coverage) passes, this PR will be great! Thanks again.

@tkow
Copy link
Contributor Author

tkow commented Dec 25, 2022

@jmcdo29

Thank you for reviews, in fact, I'm not native English speaker, so I apologize for many grammatical mistakes.

I have no idea to fix CI / send-coverage, so could I ask you how to fix it?
I may not allowed to run the github action's step (https://githubhelp.com/codacy/codacy-coverage-reporter-action/issues/33).

BTW, I really think this library is awesome and I have been making many our command line tools standalone and want it more known widely especially in my country, Japan. It can be used not for nest application but for command line tools builder. If you don't mind it, could you let me translate docs in ja locale when I have time?
And, sorry for that I make you work even though today is Xmas. I'll give you my gratitude from sponsored page though it may be little for you. Have a nice day.

@jmcdo29
Copy link
Owner

jmcdo29 commented Dec 25, 2022

I have no idea to fix CI / send-coverage, so could I ask you how to fix it?

It's just because of a missing API token for Codacy. Don't worry about it :)

@jmcdo29
Copy link
Owner

jmcdo29 commented Dec 25, 2022

If you don't mind it, could you let me translate docs in ja locale when I have time?

Go for it! So long as you don't mind helping maintain them as well when changes happen 😄 I use AstroDocs for the documentation, which has multiple language support already built in.

@tkow
Copy link
Contributor Author

tkow commented Dec 25, 2022

@jmcdo29

It's just because of a missing API token for Codacy. Don't worry about it :)

So, Is there nothing I should change in this PR?

If everything (save for the sending coverage) passes, this PR will be great! Thanks again.

Sorry for I'm not familiar to nx e2e test, but do I still have something to have to add extra configuration to do this?

Go for it! So long as you don't mind helping maintain them as well when changes happen 😄 I use AstroDocs for the documentation, which has multiple language support already built in.

Thanks! I'll try it.

@jmcdo29
Copy link
Owner

jmcdo29 commented Dec 25, 2022

So, Is there nothing I should change in this PR?

Nah, it's all good

@jmcdo29 jmcdo29 merged commit 9a25598 into jmcdo29:main Dec 30, 2022
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.

Redundant provider registered subcommands
2 participants