Skip to content

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

Closed

Conversation

navarroaxel
Copy link

I know this need more unit test cases. You think this is the way?

Copy link
Contributor

@marionebl marionebl left a 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?

@@ -0,0 +1,12 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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": {
Copy link
Contributor

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

Copy link
Author

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.

@navarroaxel navarroaxel force-pushed the camelCase branch 2 times, most recently from e71275f to 493a372 Compare September 8, 2017 13:12
@navarroaxel
Copy link
Author

Let me know if this is OK to add more unit test cases for scope, type and subject

@marionebl
Copy link
Contributor

Looks good.
The test cases for type and subject would be good to define the corresponding behaviour.

@navarroaxel
Copy link
Author

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 ?

@marionebl
Copy link
Contributor

The parser-preset option allows for non-angular headerPattern settings, making Start Case types possible - I think we should support that.

@navarroaxel
Copy link
Author

How should I change the 5 tests to pass?

@marionebl
Copy link
Contributor

marionebl commented Sep 14, 2017

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 parse to pass a headerPattern and headerCorrespondence to conventional-commits-parse allowing for whitespace in the field you want to test.

An example for parserConfiguration can be found here: https://github.com/marionebl/commitlint/blob/master/%40commitlint/cli/fixtures/parser-preset/parser-preset.js

@navarroaxel
Copy link
Author

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 :.

@marionebl
Copy link
Contributor

Landing this with changes via https://github.com/marionebl/commitlint/pull/84

@marionebl marionebl closed this Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants