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: Add ability to customise type displays #363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Sidnioulz
Copy link

This PR adds the ability to set the display characters for any action type. It works both for custom action types and for built-in ones.

Some notes:

  • I didn't know how to name things; feedback is welcome here on the name the functions should have
  • I first wanted to add a third parameter to setActionType to allow for setting both the type and the display together, but I am unsure of the consequences considering how loadAsset works in node-plop
  • The Chinese README doc is missing

@crutchcorn
Copy link
Member

I'll take a deeper dive later today, but I wanted to give initial feedback that this is one of the most comprehensive PRs I've ever received for the project.

Deeply appreciate that not only did you change the code, but you:

  • Modified existing tests
  • Added new unit tests
  • Added new E2E tests
  • Wrote docs

Honestly just all around 👏👏👏

@Sidnioulz
Copy link
Author

I'll take a deeper dive later today, but I wanted to give initial feedback that this is one of the most comprehensive PRs I've ever received for the project.

Least I could do considering the many days your project saved me! I wrote a codegen CLI by hand in the past and I can appreciate how useful Plop is. Looking forward to your review!

Copy link
Author

@Sidnioulz Sidnioulz left a comment

Choose a reason for hiding this comment

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

  • Add missing type defs in node-plop and reexport them in plop

@Sidnioulz
Copy link
Author

Hi @crutchcorn, just in case you're waiting for me to take care of the above task to do a review, I'm waiting for a review to make batch fixes to the branch 😅

@crutchcorn
Copy link
Member

Overall this code looks awesome @Sidnioulz - apologies for the massive delay in the review.

If you want to finish up the last remaining tasks I'll merge this ASAP :)

@Sidnioulz
Copy link
Author

Overall this code looks awesome @Sidnioulz - apologies for the massive delay in the review.

If you want to finish up the last remaining tasks I'll merge this ASAP :)

Thanks, I think it should be done now. I've reformatted (amended the initial commit) and added type declarations to the NodePlopAPI (separate commit).

Leaving this in your hands :-)

@Sidnioulz
Copy link
Author

Hi @crutchcorn, any chance you look at this in the coming weeks?

To the best of my knowledge, the windows-latest, 16 test was broken on main and unrelated to this branch. I've force-pushed the exact same code to re-trigger the workflow and check the test logs, as they've expired since I last committed.

Besides that, do you need help maintaining this package?

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.

2 participants