feat: allow scope-enum to also config delimiter #2407

longlho · 2021-01-15T03:03:43Z

Allow scope-enum rule to also configure delimiter regex

Description

Motivation and Context

Usage examples

// commitlint.config.js
module.exports = {};

echo "your commit message here" | commitlint # fails/passes

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

longlho · 2021-01-15T03:06:05Z

cc @escapedcat

escapedcat · 2021-01-15T03:06:33Z

Thanks @longlho ! @armano2 would you mind taking a look?

armano2 · 2021-01-15T10:59:07Z

there is few things that are missing right now,

enumRuleIsActive has to be adapted to take new configuration into account:

commitlint/@commitlint/prompt/src/library/utils.ts

Lines 82 to 96 in baf1082

    
           export function enumRuleIsActive( 
        
           	rule: RuleEntry 
        
           ): rule is [ 
        
           	string, 
        
           	Readonly< 
        
           		[RuleConfigSeverity.Warning | RuleConfigSeverity.Error, 'always', string[]] 
        
           	> 
        
           ] { 
        
           	return ( 
        
           		ruleIsActive(rule) && 
        
           		ruleIsApplicable(rule) && 
        
           		Array.isArray(rule[1][2]) && 
        
           		rule[1][2].length > 0 
        
           	); 
        
           }

same for this piece of code:

commitlint/@commitlint/prompt/src/library/get-prompt.ts

Lines 119 to 121 in baf1082

    
           const [, [, , enums]] = enumRule; 
        
           enums.forEach((enumerable) => {

currently there is no tests that validate configuration for prompt

longlho · 2021-01-15T15:29:36Z

@armano2 I made the changes. I didn't change prompt bc the new config is rather advanced to be prompted so just reserved for hand-rolling config file (if I understand it correctly).

escapedcat · 2021-01-22T07:14:32Z

For some reasons the checks here don't start/complete.
Not sure if it's related to this?:

I'm not sure what this means.
@longlho would you try to rebase master and try again? Or you have other ideas how to solve this?

longlho · 2021-01-22T14:45:19Z

hmm I rebased but also don't know what's going on

escapedcat · 2021-01-23T11:46:36Z

We had something similar before with this PR: #2224 (comment)
Does this help?

fix conventional-changelog#2108

longlho · 2021-01-23T16:12:25Z

hmm I tried but no dice it seems

escapedcat · 2021-01-25T06:45:13Z

Ugh, this is confusing. I tried google but no luck.

armano2 · 2021-01-25T06:52:54Z

@escapedcat, this is not an issue, circleci was doing ~2-3 days ago https://status.circleci.com/incidents/0jqzs9bd1ff3, you should retrigger build to fix this

armano2 · 2021-01-25T07:01:04Z

@commitlint/prompt/src/library/utils.ts

 export function enumRuleIsActive(
-	rule: RuleEntry
+	rule: RuleEntry<EnumRuleOptions>


this check is not valid, as rule, we should require RuleEntry, rn, you basically force to have correct values, than type-guard is semi useless, this works only because you force expected type mapping in getRules, and all guards are useless now

escapedcat · 2021-01-25T07:03:01Z

Can't find this PR in the list of pipelines: https://app.circleci.com/pipelines/github/conventional-changelog/commitlint
All other PRs are there.
How to re-trigger it from my side?

@commitlint/prompt/src/library/utils.ts

 ): rule is
 	| [string, Readonly<[RuleConfigSeverity, 'always']>]
-	| [string, Readonly<[RuleConfigSeverity, 'always', unknown]>] {
+	| [string, Readonly<[RuleConfigSeverity, 'always', C]>] {


armano2 · 2021-01-25T07:04:30Z

@commitlint/prompt/src/library/utils.ts

+export function getRules<C = unknown>(
+	prefix: string,
+	rules: QualifiedRules
+): RuleEntry<C>[] {


this function returns array of unknown rules and you can't know what types thy are going to be, all what you know is that prefix stats with something

armano2 · 2021-01-25T07:10:24Z

@commitlint/prompt/src/library/utils.ts

+		!!(config = rule[1][2]) &&
+		(!enumConfigIsExtendable(config)
+			? config.length > 0
+			: Array.isArray(config.values) && config.values.length > 0)


Suggested change

!!(config = rule[1][2]) &&

(!enumConfigIsExtendable(config)

? config.length > 0

: Array.isArray(config.values) && config.values.length > 0)

(Array.isArray(rule[1][2])

? rule[1][2].length > 0

: typeof rule[1][2] === 'object' &&

'values' in rule[1][2] &&

rule[1][2].length > 0)

armano2 · 2021-01-25T07:17:34Z

@commitlint/prompt/src/library/utils.test.ts

 	};

-	let enumRule = getRules('type', rules)
+	let enumRule = getRules<EnumRuleOptions>('type', rules)


all of those forced types should be removed

armano2 · 2021-01-25T07:18:20Z

@commitlint/prompt/src/library/utils.test.ts

-	const rules: any = {
+	const rules: Record<string, RuleEntry<EnumRuleOptions>[1]> = {


armano2 · 2021-01-25T07:19:21Z

Can't find this PR in the list of pipelines: https://app.circleci.com/pipelines/github/conventional-changelog/commitlint
All other PRs are there.
How to re-trigger it from my side?

easiest will be pushing something to this branch, as it should retrigger build

i added some comments about type guards, type guard should change type only for stuff that actualy is validated in function, we can't have getRules return X rule as it can cause issues -> this function returns all reles that are prefixed with something

armano2 · 2021-01-25T07:24:52Z

@commitlint/prompt/src/library/get-prompt.ts

@@ -118,7 +121,7 @@ export default function getPrompt(
 		if (enumRule) {
 			const [, [, , enums]] = enumRule;

-			enums.forEach((enumerable) => {
+			(enums as string[]).forEach((enumerable) => {


you should not do typecasts as types should be correct here, you can do something like this

Suggested change

(enums as string[]).forEach((enumerable) => {

(Array.isArray(enums) ? enums : enums.values).forEach((enumerable) => {

armano2 · 2021-01-25T07:33:25Z

@commitlint/prompt/src/library/utils.ts

+function enumConfigIsExtendable(
+	config?: EnumRuleOptions
+): config is EnumRuleExtendableOptions {
+	return !Array.isArray(config);
+}


this guard is invalidas its only check is not any[]

escapedcat · 2021-03-26T04:23:47Z

@longlho would you be able to integrate aramanos feedback? If the checks still won't pass we can do the checks locally and merge it in anyways. Not sure what's the issue with circleci here or what we could do to make this work.

longlho · 2021-03-26T04:31:24Z

sry I haven't got time to get back to this PR :(

escapedcat · 2021-03-26T04:34:02Z

No worries, just re-open it once you find some time ❤️

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimeters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarious by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we initially anticipated. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimeters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarious by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we initially anticipated. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimeters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarious by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we initially anticipated. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimeters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarious by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we wanted in the first place. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimiters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarious by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we wanted in the first place. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimiters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarios by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we wanted in the first place. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimiters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarios by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we wanted in the first place. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

@bbogdanov

When doing the research we missed that commitlint has some multi-scope predefined delimiters and what we are currently using is in conflict with commitlint expectations. There is a PR in commitlint repo to fix these scenarios by letting people provide a custom delimiters for scopes but the owner is not actively working on that. @bbogdanov will try to join and push that PR to get in for their next release and unblock us to get back to what we wanted in the first place. conventional-changelog/commitlint#2407 Signed-off-by: Bogdan Bogdanov <[email protected]>

longlho force-pushed the delimiter branch from eba35b0 to a4c9f0d Compare January 15, 2021 03:05

escapedcat requested a review from byCedric January 15, 2021 03:06

longlho force-pushed the delimiter branch from a4c9f0d to 5697043 Compare January 15, 2021 03:07

longlho force-pushed the delimiter branch from 5697043 to 39095fb Compare January 15, 2021 15:37

longlho force-pushed the delimiter branch from 39095fb to 68da942 Compare January 22, 2021 14:44

feat: allow scope-enum to also config delimiter

7993168

fix conventional-changelog#2108

longlho force-pushed the delimiter branch from 68da942 to 7993168 Compare January 23, 2021 16:11

longlho closed this Jan 25, 2021

longlho reopened this Jan 25, 2021

armano2 reviewed Jan 25, 2021

View reviewed changes

@commitlint/prompt/src/library/utils.ts

): rule is

| [string, Readonly<[RuleConfigSeverity, 'always']>]

| [string, Readonly<[RuleConfigSeverity, 'always', unknown]>] {

| [string, Readonly<[RuleConfigSeverity, 'always', C]>] {

This comment was marked as outdated.

Sign in to view

armano2 reviewed Jan 25, 2021

View reviewed changes

escapedcat closed this Mar 26, 2021

bbogdanov mentioned this pull request Jun 17, 2021

fix(commitlint): adding another delimiter for scopes vmware-archive/clarity#6085

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow scope-enum to also config delimiter #2407

feat: allow scope-enum to also config delimiter #2407

longlho commented Jan 15, 2021 •

edited

Loading

longlho commented Jan 15, 2021

escapedcat commented Jan 15, 2021

armano2 commented Jan 15, 2021

longlho commented Jan 15, 2021

escapedcat commented Jan 22, 2021

longlho commented Jan 22, 2021

escapedcat commented Jan 23, 2021

longlho commented Jan 23, 2021

escapedcat commented Jan 25, 2021

armano2 commented Jan 25, 2021

armano2 Jan 25, 2021 •

edited

Loading

escapedcat commented Jan 25, 2021

This comment was marked as outdated.

armano2 Jan 25, 2021

armano2 Jan 25, 2021 •

edited

Loading

armano2 Jan 25, 2021

armano2 Jan 25, 2021

armano2 commented Jan 25, 2021 •

edited

Loading

armano2 Jan 25, 2021

armano2 Jan 25, 2021

escapedcat commented Mar 26, 2021

longlho commented Mar 26, 2021

escapedcat commented Mar 26, 2021

-		!!(config = rule[1][2]) &&
-		(!enumConfigIsExtendable(config)
-			? config.length > 0
-			: Array.isArray(config.values) && config.values.length > 0)
+		(Array.isArray(rule[1][2])
+			? rule[1][2].length > 0
+			: typeof rule[1][2] === 'object' &&
+			  'values' in rule[1][2] &&
+			  rule[1][2].length > 0)

		const rules: any = {
		const rules: Record<string, RuleEntry<EnumRuleOptions>[1]> = {

	(enums as string[]).forEach((enumerable) => {
	(Array.isArray(enums) ? enums : enums.values).forEach((enumerable) => {

feat: allow scope-enum to also config delimiter #2407

feat: allow scope-enum to also config delimiter #2407

Conversation

longlho commented Jan 15, 2021 • edited Loading

Description

Motivation and Context

Usage examples

How Has This Been Tested?

Types of changes

Checklist:

longlho commented Jan 15, 2021

escapedcat commented Jan 15, 2021

armano2 commented Jan 15, 2021

longlho commented Jan 15, 2021

escapedcat commented Jan 22, 2021

longlho commented Jan 22, 2021

escapedcat commented Jan 23, 2021

longlho commented Jan 23, 2021

escapedcat commented Jan 25, 2021

armano2 commented Jan 25, 2021

armano2 Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

escapedcat commented Jan 25, 2021

This comment was marked as outdated.

armano2 Jan 25, 2021

Choose a reason for hiding this comment

armano2 Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

armano2 Jan 25, 2021

Choose a reason for hiding this comment

armano2 Jan 25, 2021

Choose a reason for hiding this comment

armano2 commented Jan 25, 2021 • edited Loading

armano2 Jan 25, 2021

Choose a reason for hiding this comment

armano2 Jan 25, 2021

Choose a reason for hiding this comment

escapedcat commented Mar 26, 2021

longlho commented Mar 26, 2021

escapedcat commented Mar 26, 2021

longlho commented Jan 15, 2021 •

edited

Loading

armano2 Jan 25, 2021 •

edited

Loading

armano2 Jan 25, 2021 •

edited

Loading

armano2 commented Jan 25, 2021 •

edited

Loading