nest-cli icon indicating copy to clipboard operation
nest-cli copied to clipboard

feat: initial support for case type selection

Open espoal opened this issue 2 years ago • 16 comments

Summary

This is the second of a multi PR effort aiming at supporting custom name casing conventions, as detailed here.

  1. The first PR attempts to update @nestjs/schematics to accept a caseType option when generating a new file.
  2. In this PR we add a caseNaming field to GenerateOptions
  3. This is the PR to document the new option
  4. Future PRs will extend this feature in term of supported actions, tests and configuration

For now I decided to support only controllers, to reduce the changes size and make reviewing easier. If the approach will be approved I will make sure to extend it to other objects and to add more tests.

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Currently @nestjs/cli uses kebab-or-snake case, as mentioned in this issue.

Issue Number: 462

What is the new behavior?

The goal is to add a caseNaming option to nest-cli.json so that generated files will follow the desired name casing convention.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other informations

How to run

  • Install deps and build:
npm install
npm run build
  • Build updated @nestjs/schematics Clone this branch, and do:
npm install
npm run build
  • Install the updated @nestjs/schematics package Copy the dist folder in @nestjs/nest-cli deps: nest-cli/node_modules/@nestjs/schematics/dist/

  • Run it with one of those commands: node bin/nest.js new test-nest -s --caseNaming=camel to generate a custom nest-cli.json node bin/nest.js g co keb-pap in a folder with the correct nest-cli.json

espoal avatar Jul 31 '23 15:07 espoal

Hey @micalevisk can I help you with anything? I would love a pair programming session to review the pr together and see how to improve it

espoal avatar Aug 15 '23 13:08 espoal

@espoal I think it would make sense to add the --caseNaming option to the new command that creates a new NestJS application. Would you agree?

mwanago avatar Aug 15 '23 19:08 mwanago

yeah, instead of adding that new option to generate command, we can move it to new and to the configuration file (nest-cli.json)

micalevisk avatar Aug 15 '23 20:08 micalevisk

@micalevisk Since we usually want to keep the same naming convention throughout the project, storing this option in nest-cli.json sounds like a great idea

mwanago avatar Aug 15 '23 20:08 mwanago

@mwanago Let's split the problem in two parts:

  • My main concern is if my approach of decorating SchemaOption is a good idea, or if maybe people see a better approach.
  • I would totally be ok to use the new command. Could you make an example of how a CLI command would be using that? And where the nest-cli.json is parsed?

espoal avatar Aug 17 '23 06:08 espoal

@espoal

I would totally be ok to use the new command. Could you make an example of how a CLI command would be using that?

it would be nothing more than a new option (--caseNaming <option>) for the new command that will define the value of the caseNaming option (defaults to "kebab") of the generated nest-cli.json (I guess at GenerateOptions interface here: https://github.com/nestjs/nest-cli/blob/48939b57814f0353e1759fcabb85fe401b39e793/lib/configuration/configuration.ts#L76C18-L76C33)

Then, when using the generate command, that option will be taken into account just like it does for the spec option which uses the value from nest-cli.json.

In this case, we won't have --caseNaming for others commands.

micalevisk avatar Aug 27 '23 19:08 micalevisk

Sorry for the late reply but I was swamped with my day work. The PR is far from perfect, but I think I'm slowly getting closer to what you meant @micalevisk . Some questions:

  • I still cannot change the suffix of the file (KebPap.controller.ts instead of KebPapController.ts). Any advice on how to proceed?
  • Now I correctly use the nest-cli.json but are we sure we don't want to leave the command line option? Now we have both.
  • I cannot see why the pipeline is failing. Locally it seems to work. Suggestions on how to fix it?

And a slightly bigger topic:

  • Now when we use the new command the file don't respect the passed convention, because they're taken from a template. The solution IMHO would be to update the new command to use a sequence of commands (configuration, generate ....) instead of the templates. But this seems like a big topic, so maybe you have suggestions.

cc: @mwanago

espoal avatar Sep 29 '23 13:09 espoal

are we sure we don't want to leave the command line option?

if we can make it work without adding the options to the generate command, then yes. Less code to maintain; less options to know about. We can add them in the futurue if needed.

I cannot see why the pipeline is failing

Does not seems to be releated to your changes.

Now when we use the new command the file don't respect the passed convention, because they're taken from a template

I didn't follow. The project generated by new is too simple. The files will have the same name regardless of the convention because they have only one word. Here I'm talking about source files, not configuration (json) ones.


Also, convert this PR to draft until you're done. It helps us to understand when we can start the code review

micalevisk avatar Sep 29 '23 13:09 micalevisk

Thanks Micael for the quick feedback, I really appreciate it. My brain is fried atm from a long week, but I will commit the small changes you suggested and get back on this next week.

espoal avatar Sep 29 '23 14:09 espoal

if we can make it work without adding the options to the generate command, then yes. Less code to maintain; less options to know about. We can add them in the future if needed.

I think it's not too much code, but I also think that smaller PRs are better. Let's keep the flag for a future PR.

The project generated by new is too simple.

That's a fair point

Also, convert this PR to draft until you're done.

I still don't think the PR is perfect, but perfection is also the worst enemy of the good, so I would like to ask you @micalevisk to give another look.

Things I would like to do after this review:

  • [x] Decide which case formats we support
  • [x] Add documentation
  • [x] Add more tests

P.S.: I managed to fix the pipeline :partying_face:

espoal avatar Oct 05 '23 12:10 espoal

In the end I decided that the case we should support are:

  • snake
  • kebab
  • camel
  • pascal

I would propose to drop the kebab-or-snake case for the reasons outlined here. Let me know if you would prefer to keep it for now and remove it in a future PR.

@micalevisk I would love to ask you for another review

espoal avatar Oct 17 '23 07:10 espoal

Hey @micalevisk is there anything I could do to speed up the merging of this PR?

espoal avatar Nov 06 '23 16:11 espoal

@espoal nop. I'll try to review this ASAP

micalevisk avatar Nov 06 '23 16:11 micalevisk

Thanks, I apologies to bother you but I'm really excited about my first nestjs PR :heart:

espoal avatar Nov 06 '23 16:11 espoal