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

Lint rules #980

Open
theKashey opened this issue May 24, 2021 · 6 comments
Open

Lint rules #980

theKashey opened this issue May 24, 2021 · 6 comments

Comments

@theKashey
Copy link

Description

Thanks for such a great friend. We are together for many years, and we have developed some habits how we live together, and what we can expect from each other.

Those habits sometimes lead to a bad situation when a friend lets a friend down 😢

Steps to reproduce

  • how it started
{
  "*.js": "this works",
  "*.scss": "this works",
}
  • how it ended
{
  "*.{js,jsx,ts,tsx}": "this works",
  // let's be consistent in the pattern!
  "*.{scss}": "this dont",
}

The issue is not bound to the lint-staged and more about implementation detail of minimatch, but we spend ~month trying to understand why our styles are not linted on commit.

We have 6 lint-staged rules, 5 never worked.

Proposed solution

Report a wrong match in the configuration.

@okonet
Copy link
Collaborator

okonet commented May 24, 2021

🤔 I understand your frustration but I’m not sure how to solve this. Is here a way to tell that a glob is invalid from minimatch perspective? Feel free to propose a solution!

@theKashey
Copy link
Author

theKashey commented May 24, 2021

Look like I found something "acceptable" for me:

if(pattern.includes('{')) {
  if(braces(pattern, {expand: true}.length===1) {
    throw new Error(pattern + "should be written as "+pattern.replace(/[\{\}]/g,''))
  } 

I can make nessesary amends. The question is how NOT to make this a breaking change, as some people might have preexisting wrong configuration.

@iiroj
Copy link
Member

iiroj commented May 24, 2021

For what it's worth, I asked about this earler here: micromatch/braces#32

@okonet
Copy link
Collaborator

okonet commented May 24, 2021

I think we’re fine with a breaking change but I don’t see it as such since it’s leading to a working configuration. Maybe instead of throwing we could print a warning? In this case it’s not a breaking change. But as I said I’m fine with a breaking one too if it leads to better DX.

@theKashey
Copy link
Author

@iiroj - thanks for validating my concern. I'll prepare nessesary changes during weekends.

@iiroj
Copy link
Member

iiroj commented Jul 13, 2021

I opened this PR to try fixing this issue by detecting and removing these unnecessary/incorrect braces, and also warning the user:

#992

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

No branches or pull requests

3 participants