feat: initial support for case type selection
Summary
This is the second of a multi PR effort aiming at supporting custom name casing conventions, as detailed here.
- The first PR attempts to update
@nestjs/schematicsto accept acaseTypeoption when generating a new file. - In this PR we add a
caseNamingfield toGenerateOptions - This is the PR to document the new option
- 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/schematicsClone this branch, and do:
npm install
npm run build
-
Install the updated
@nestjs/schematicspackage Copy thedistfolder in@nestjs/nest-clideps:nest-cli/node_modules/@nestjs/schematics/dist/ -
Run it with one of those commands:
node bin/nest.js new test-nest -s --caseNaming=camelto generate a customnest-cli.jsonnode bin/nest.js g co keb-papin a folder with the correctnest-cli.json
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
I think it would make sense to add the --caseNaming option to the new command that creates a new NestJS application. Would you agree?
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
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 Let's split the problem in two parts:
- My main concern is if my approach of decorating
SchemaOptionis a good idea, or if maybe people see a better approach. - I would totally be ok to use the
newcommand. Could you make an example of how a CLI command would be using that? And where thenest-cli.jsonis parsed?
@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.
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.tsinstead ofKebPapController.ts). Any advice on how to proceed? - Now I correctly use the
nest-cli.jsonbut 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
newcommand the file don't respect the passed convention, because they're taken from a template. The solution IMHO would be to update thenewcommand 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
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
newcommand 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
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.
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:
In the end I decided that the case we should support are:
snakekebabcamelpascal
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
Hey @micalevisk is there anything I could do to speed up the merging of this PR?
@espoal nop. I'll try to review this ASAP
Thanks, I apologies to bother you but I'm really excited about my first nestjs PR :heart: