Skip to content
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

fix slash in scope #291 #529

Merged

Conversation

escapedcat
Copy link
Member

@escapedcat escapedcat commented Dec 23, 2018

Tried to narrow this down.

Description

Using this scope Modules\Graph with the pascal-case rule fails.

In @commitlint/rules/src/scope-case.js:
const r = ensure.case(scope, check.case); this returns false for such case.

First I only try to add a test-case which would be suitable for this situation. If this is correct we could chech where a fix in case.js coudl be implemented.

Currently thsi test fails:

✖ case › true for Modules/Graph on pascal-case

@escapedcat
Copy link
Member Author

@marionebl
@byCedric
I tried to dig into https://github.com/marionebl/commitlint/issues/291
Narrowed it down and tried to first write a test which shoudl cover that case and shoudl work in the future.
Feedback appreciated :)

@byCedric
Copy link
Member

byCedric commented Dec 30, 2018

I like the tests-first approach! 👍 I've also dug a bit deeper into @commitlint/ensure to find out why this is happening. Well, I think I have found something. In the ensure.case function, I see that it cleans some special chars and detects empty/numeric values. But I also noticed that Lodash is being used here, which might explain the issue here.

Lodash camelCase is not treating string feature/my-branch as separate words. Instead, it strips out the dash / and converts the letters. So, camelCase('feature/my-branch') === 'featureMyBranch' instead of 'feature/myBranch'

A solution that comes to mind is trying to split the branch name based on either one of the dashes (/ or \). By splitting them, we can still use Lodash without causing this side effect. I think this behavior matches the folder-like structure some Git clients use for branch names. What do you guys think?

@byCedric
Copy link
Member

byCedric commented Jan 1, 2019

I created the solution I explained earlier, wanted to see if it could work. And well, all tests are in the green. 😄

Replace in file ensure/src/case.js, line 11 the transformed code with this:

	const delimiters = /(\/|\\)/g;
	const transformed = input.split(delimiters)
		.map(segment => delimiters.test(segment) ? segment : toCase(segment, target))
		.join('');

This expects either a / or \ character and splits it into separate words. Because it's split, underscore handles it as we would expect. If you only want to allow / as a delimiter, you can remove the delimiters regex, remove the delimiters test, use .split('/') and use .join('/').

@escapedcat
Copy link
Member Author

Mind blown. You want to add that code? Added you as a collaborator on my fork. Or you branch out from this one.

@byCedric
Copy link
Member

byCedric commented Jan 2, 2019

Haha, nice 😄 I've added the code! Is this something that we can use or do you know other solutions?

@escapedcat escapedcat changed the title WIP: test(rules): add test for slash in scope #291 fix slash in scope #291 Jan 11, 2019
@marionebl marionebl merged commit b2b63e5 into conventional-changelog:master Jan 27, 2019
@escapedcat escapedcat deleted the fix/291_slash-in-scope branch February 5, 2023 13:03
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.

None yet

3 participants