-
Notifications
You must be signed in to change notification settings - Fork 395
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: initial support for case type selection #2208
base: master
Are you sure you want to change the base?
Conversation
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 |
yeah, instead of adding that new option to |
@micalevisk |
@mwanago Let's split the problem in two parts:
|
it would be nothing more than a new option ( Then, when using the In this case, we won't have |
Sorry for the late reply but I was swamped with my day work.
And a slightly bigger topic:
cc: @mwanago |
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.
That's a fair point
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:
P.S.: I managed to fix the pipeline 🥳 |
{ | ||
description: 'should manage classified string option value name', | ||
option: 'name', | ||
input: 'MyApp', | ||
expected: 'my-app', | ||
}, | ||
{ | ||
/*{ | ||
description: 'should manage parenthesis string option value name', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Atm we don't really respect this. In package.json
we have the right name my-(app)
, but at least on linux the folder is actually called ./'my-(app)'
(note the dashes). Same for the [
character.
The test fails because the case conversion function performs sanitization, but I could fix it with this:
case 'kebab':
return kebabCase(str, { keep: ['[', ']', '(', ')'] })
@@ -38,19 +38,19 @@ describe('Schematic Option', () => { | |||
input: 'myApp', | |||
expected: 'my-app', | |||
}, | |||
{ | |||
/*{ | |||
description: 'should allow underscore string option value name', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this could be a source of confusion: if I specify the kebab
case, I would expect the convention to be respected.
The code atm reflect this behavior, hence the test fails because my_app
turns to my-app
.
In the end I decided that the case we should support are:
I would propose to drop the @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 ❤️ |
} | ||
}; | ||
|
||
export const formatString = (str: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you elaborate here in the code on what this function is supposed to do?
github copilot can help :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, I'm surprised copilot understood immediately the code. I'm starting to question reality :D
Added some JsDoc. Basically this function escapes parenthesis which will then later be removed.
I see here an opportunity to create @nestjs/utils
or @nestjs/stringUtils
because this code is shared across projects, and I feel it would be beneficial to centralize it so that it's easier to maintain and can be reused by plugin developers. I didn't do it to keep the scope limited, but it shouldn't be too hard to do and I would be up for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I updated the same code also in @nestjs/schematics
Summary
This is the second of a multi PR effort aiming at supporting custom name casing conventions, as detailed here.
@nestjs/schematics
to accept acaseType
option when generating a new file.caseNaming
field toGenerateOptions
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:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently
@nestjs/cli
useskebab-or-snake
case, as mentioned in this issue.Issue Number: 462
What is the new behavior?
The goal is to add a
caseNaming
option tonest-cli.json
so that generated files will follow the desired name casing convention.Does this PR introduce a breaking change?
Other informations
How to run
@nestjs/schematics
Clone this branch, and do:
Install the updated
@nestjs/schematics
packageCopy 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 customnest-cli.json
node bin/nest.js g co keb-pap
in a folder with the correctnest-cli.json