-
Notifications
You must be signed in to change notification settings - Fork 934
feat(core): add kebab-case, camelCase, PascalCase options #72
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
Conversation
8d62811
to
ce176a5
Compare
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.
Great work and some comments from me. Could you make sure to remove the yarn.lock file?
.idea/commitlint.iml
Outdated
@@ -0,0 +1,12 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
Could you remove those and add .idea
to .gitignore
?
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.
Why do you commit the .vscode? We can add **/.*/
to ignore all the IDEs generated files.
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.
The .vscode
directory contains config I'd like to share with other vscode users. Afaik the .idea
directory is specific to each machine
switch (target) { | ||
case 'camelcase': | ||
normalized = camelCase(input); |
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'd prefer each case returning on its own, e.g. return input === camelCase(input)
. That would avoid thelet
, too.
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.
It's not logic duplicated? I want avoid that putting the equality compare only one time.
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.
Given all of them are strict equality checks I'd choose this direction, yes.
package.json
Outdated
@@ -56,6 +56,9 @@ | |||
"name": "Mario Nebl", | |||
"email": "[email protected]" | |||
}, | |||
"dependencies": { |
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.
Please remove, lodash
is a dependency of @commitlint/core
already
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.
Oh. I didn't know that. This is my first time with lerna
.
e71275f
to
493a372
Compare
Let me know if this is OK to add more unit test cases for scope, type and subject |
Looks good. |
493a372
to
053c230
Compare
5 tests are failing becaue the type part does not support Start Case. What we can do with that? Support spaces on the type or do not support Start Case as a valid option for type ? |
053c230
to
b63ab97
Compare
The parser-preset option allows for non-angular |
How should I change the 5 tests to pass? |
I am completely swamped with day-work at the moment, sorry. I'll have time to provide an example next week the soonest. You'll want to use the second parameter of An example for parserConfiguration can be found here: https://github.com/marionebl/commitlint/blob/master/%40commitlint/cli/fixtures/parser-preset/parser-preset.js |
The test are failing because the type is empty with Start Case so the type empty is succeed with every option (i.e. camelCase). Maybe the defaultChangelogOpts should be changed to fail with spaces before the |
b63ab97
to
2d3be3c
Compare
Landing this with changes via https://github.com/marionebl/commitlint/pull/84 |
I know this need more unit test cases. You think this is the way?