Create CLI questionnaire #57
Conversation
|
This is lookin' great so far @macklinu 😍 also the first time I've seen a draft PR from a fork, I dig it. Thanks for doing this and for the super helpful hints above ❤️ |
There was a problem hiding this comment.
Alright @JasonEtco, getting much closer to a final review. I think one more review of the architecture and tests here would be helpful to make sure I'm still going in the right direction. If anything is confusing, please comment and I can explain further (both with PR comments and perhaps some code comments to help explain some of the mocking stuff, if that'd help).
Based on this PR's feedback, I should be able to finish this up likely early next week. Thanks! 🎨
|
I'm about one pass away from requesting a full review. Should be able to take another look today or tomorrow. 👍 |
There was a problem hiding this comment.
@JasonEtco this is ready for review! 😸 Left some inline comments, and I also updated the PR description to further explain my decisions and some manual test cases I've run to test this feature locally.
|
|
||
| const createAction = require('./create-action') | ||
|
|
||
| createAction(process.argv.slice(2)) |
There was a problem hiding this comment.
This is now the main CLI entrypoint - also defined in package.json.
|
This is amazing @macklinu. Thanks for the time and effort you've put into this work ❤️ I did some light reviewing, but I'll dedicate some time soon for a full review - hopefully we'll be able to wrap this up and get it merged next week 🤞 |
Co-Authored-By: macklinu <macklinu@users.noreply.github.com>
Co-Authored-By: macklinu <macklinu@users.noreply.github.com>
Using Promise.all is better than Array.prototype.forEach for the purposes of writing multiple files to disk.
macklinu
force-pushed
the
cli-questionnaire
branch
from
97e1417
to
fc3dd0b
Compare
|
@JasonEtco rebased against master and pushed up a commit to address the most recent PR comments. 👍 |
|
Ok so I've given this a real review @macklinu - its really solid, the experience is 👌. The one thing I want to improve is the logging - we're using
Unfortunately, this totally borks in tests - I've tracked it down to be caused by the mock |
|
I’ll follow up on this today - thanks!
|
|
|
||
| exports[`creates project with labels passed to Dockerfile from questionnaire: package.json 1`] = ` | ||
| { | ||
| "name": "__tmp/my-project-name", |
There was a problem hiding this comment.
Not sure this is desired and might be an implementation bug? Check out this test - essentially it runs:
npx actions-toolkit __tmp/my-project-name
Which would create a folder a couple of levels deep. Might be a bug or perhaps a side-effect of my test implementation and would love another set of eyes on this. 👀 ❤️
There was a problem hiding this comment.
Interesting... so when I try running that, it gives me a "__tmp doesn't exist" when it tries to make the my-project-name dir (which makes sense).
That said, I think its fine? Thanks for calling it out 🙏
There was a problem hiding this comment.
From my perspective, this is good to 🚢 thanks a million for your hard work @macklinu - seriously, this is a great enhancement and took a lot of effort ❤️ Because of the complexity of the PR, I'll hold off until you give me a 👍 to merge, just to make sure there's nothing else you're looking to do. 🎉
|
Nothing else I’m looking to do here - thanks for your help in collaborating on this feature with me! 😀👏 |
Please squash and merge this PR when ready to avoid merging so many intermediate commits. ❤️
Why?
Resolves #54, and because it's fun to improve developer experience. 😄
How?
bin/cli.jsis now the CLI entrypoint. It requiresbin/create-action.js, which is a Promise that parses CLI arguments, reads files, writes files, and prompts the user for information to fill out the required metadata for their GitHub Action.bin/create-actions.jshas been split out into its own file so that the Promise function is unit testable on its own.This PR also includes the
scripts/update-feather-icons.js, which pulls a list of feather icon names that can be selected during the CLI prompt.I suggest pulling down this branch and playing around with the new CLI experience - feedback is definitely welcome on UI/messaging in the CLI.
Test scenarios:
npx actions-toolkitwithout args should display help messagenpx actions-toolkit --helpshould display help messagenpx actions-toolkit <directory-name>should start the CLI prompt to ask for name, description, etc.npm installit successfully