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

Slash in subject results in undesired results #572

Closed
1 of 4 tasks
esagawe opened this issue Feb 6, 2019 · 7 comments
Closed
1 of 4 tasks

Slash in subject results in undesired results #572

esagawe opened this issue Feb 6, 2019 · 7 comments
Labels

Comments

@esagawe
Copy link

esagawe commented Feb 6, 2019

I use this config:

{
  "extends": ["@commitlint/config-conventional"],
  "rules": {
    "scope-case": [0],
    "subject-case": [2, "always", "sentence-case"]
  }
}

If I write something like this, then the cli throws an error: chore: Update @angular/core.

⧗   input: chore: Update @angular/core
✖   subject must be sentence-case [subject-case]
✖   found 1 problems, 0 warnings

Expected Behavior

The subject should be valid.

Current Behavior

The toCase function in case.js will be called two times:

  • Update @angular
  • core

So there is somewhere a split by the slash, which results in a validation error because core starts with an lower case.

Affected packages

  • cli
  • core
  • prompt
  • config-angular

Possible Solution

I think there should be no split in the subject. Maybe there is a connection to #291

@byCedric
Copy link
Member

byCedric commented Feb 7, 2019

Ah I see, thanks for the report! I think this might be related to a fix we implemented to allow slashes in the scope part (#341). We could change it a bit to allow slashes like that before the type-subject delimiter. Or we might be able to solve this using options provided to ensure/toCase method, like { allowSlashes: true|false }. What do you think about this @marionebl @escapedcat?

Let me create a branch/pr with the scenario described here, after that we can find a way to fix this.

@escapedcat
Copy link
Member

[...] allow slashes like that before the type-subject delimiter.

I feel like this might be a way to go.
Adding just another option to take care of to support another edge-case seems cumbersome to me.

So this would still be checked in the future:

chore(your/component): things and @angular/core

If sentence-case is chosen it should be corrected to:

chore(Your/Component): Things and @angular/core

Correct?

@byCedric
Copy link
Member

byCedric commented Feb 8, 2019

Adding just another option to take care of to support another edge-case seems cumbersome to me.

Yeah, I feel the same. But it's one of the options available 😄

Correct?

Yeah, exactly! I should definitely add some examples next time too 😄

@byCedric
Copy link
Member

byCedric commented Feb 9, 2019

Hmm, wait, I don't think our solution would work @escapedcat. The idea works fine for ensure with the full commit. But I double checked the lint package, how it executes the rules and what data they are passed. Turns out, the scope only gets the scope and subject gets the subject only.

One thing I'll try out now is to move the delimiter-split-logic to the scope-case rule only. With that, we remove the misbehaving logic and make it scope only. I think that also helps to keep the parsed result (and settings, e.g. delimiter) as a single source of truth without having hardcoded regexes.

@byCedric
Copy link
Member

byCedric commented Feb 9, 2019

I think that actually did the trick! Because the slashes are very scope-specific, in hindsight, it would be better to implement that in the scope-case-rule itself. PR #574 is ready for review!

@byCedric
Copy link
Member

Fix from #574 is merged and released in 7.5.2

@esagawe
Copy link
Author

esagawe commented Feb 12, 2019

Thank you guys, works well for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants