[Feature Request] Set 'allowEmptyScopes' based on commitlint config

[jaklan]

💭 Describe the feature

Respect commitlint option about empty scopes by default, when there's no option set for cz-git explicitly:

  rules: {
    "scope-empty": [2, "never"]
  }

https://commitlint.js.org/#/reference-rules?id=scope-empty

💡 Proposed Solution

Fallback to commitlint config when the cz-git one is not specified directly.

If the commitlint setting is in the warning mode (1) instead of the error one (2), we could either ignore it or maybe provide some additional message.

[Zhengqbbb]

I feel like this is a bug, I need to locate it further.

[Zhengqbbb]

The latest version 🌟(v1.2.4)🌟 should be resolve this issue request , please have a try. 💪
👀 Let me know if there are still any issues. Thanks for reporting it! 🎉

[jaklan]

@Zhengqbbb I just tried it and I think it still doesn't work :/

Sample configuration:

  extends: ["@commitlint/config-conventional"],
  rules: {
    "scope-empty": [2, "never"],
  },

when I run cz, I still see:

❯ gcz
cz-cli@4.2.4, cz-git@1.2.4

? Select the type of change that you're committing: feat:     A new feature
? Denote the SCOPE of this change: 
  ──────────────
  custom 
❯ empty 

[jaklan]

@Zhengqbbb or actually I think the problem is with allowEmptyScopes option per se - now I tried with no extends:, with all commitlint rules removed and with allowEmptyScopes: false added - and I still see exactly the same prompt as above.

[jaklan]

@Zhengqbbb one more issue I discovered (it's not related to the above bug):

 allowEmptyScopes: pkgConfig.allowEmptyScopes ?? clPromptConfig.allowEmptyScopes ?? !ruleIsActive(clConfig?.rules?.["scope-empty"] as any),

When you pass "scope-empty": [2, "never"] to !ruleIsActive(clConfig?.rules?.["scope-empty"] as any), then the result of ruleIsActive would be true, so the negated one would be false, so empty scopes would be disallowed - and that's fine.

But - when I pass "scope-empty": [2, "always"] - the result would be exactly the same, because we don't check the value of second element in array in ruleIsActive function - that's actually done in ruleIsNotApplicable, which is not used here.

[jaklan]

@Zhengqbbb I think I found the issue:

cz-git/packages/cz-git/src/until/until.ts

Lines 59 to 72 in aa0e8fb

	const filterCustomEmptyByOption = (
	target: {
	name: string;
	value: any;
	}[],
	allowCustom = true,
	allowEmpty = true
	) => {
	if (!Array.isArray(target) || target.length === 3 || target.length === 4) {
	return allowCustom ? target : target.filter((i) => i.value !== "___CUSTOM___");
	}
	target = allowCustom ? target : target.filter((i) => i.value !== "___CUSTOM___");
	return allowEmpty ? target : target.filter((i) => i.value !== false);
	};

• when you don't define scopes - you get:

? Denote the SCOPE of this change: 
  ──────────────
  custom 
❯ empty 

• when you define single scope like:

scopes: [{ name: "scope1", value: "scope1" }],

you get:

? Denote the SCOPE of this change: (Use arrow keys or type to search)
❯ scope1 
  ──────────────
  custom 
  empty 

but when you define 2 or more scopes:

scopes: [{ name: "scope1", value: "scope1" }, { name: "scope2", value: "scope2" }],

you get:

? Denote the SCOPE of this change: (Use arrow keys or type to search)
❯ scope1 
  scope2 
  ──────────────
  custom 

So the problem is the target.length check in the below if statement:

   if (!Array.isArray(target) || target.length === 3 || target.length === 4) { 
     return allowCustom ? target : target.filter((i) => i.value !== "___CUSTOM___"); 

leads to wrong results, because it doesn't check allowEmpty at all. I'm not sure what was the exact goal of target.length === 3 || target.length === 4 check, but it seems to be just too naive.

I just wonder if that if statement is actually needed at all? Implementing the function without it wouldn't be enough?

 const filterCustomEmptyByOption = ( 
   target: { 
     name: string; 
     value: any; 
   }[], 
   allowCustom = true, 
   allowEmpty = true 
 ) => { 
   target = allowCustom ? target : target.filter((i) => i.value !== "___CUSTOM___"); 
   return allowEmpty ? target : target.filter((i) => i.value !== false); 
 }; 

[Zhengqbbb]

@jaklan Sorry to trouble you. At the same time I did not find out. You are right. This implementation does need to change.

The main reason for this is due to the easing strategy. I haven't been very strict with empty before because I thought I might need to provide an exit for the next step.