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 isPathIgnored regression for .stylelintignore files #2833

Closed
proProbe opened this issue Aug 24, 2017 · 26 comments · Fixed by #3606
Closed

Fix isPathIgnored regression for .stylelintignore files #2833

proProbe opened this issue Aug 24, 2017 · 26 comments · Fixed by #3606
Labels
status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule

Comments

@proProbe
Copy link

proProbe commented Aug 24, 2017

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

There is a bug where the API apparently doesnt read .stylelintignore-files. CLI works fine but not the API.

Which rule, if any, is this issue related to?

no rule

What CSS is needed to reproduce this issue?

No actual css

What stylelint configuration is needed to reproduce this issue?

doesnt matter

Which version of stylelint are you using?

^8.0.0

How are you running stylelint: CLI, PostCSS plugin, Node API?

In vscode with
https://github.com/shinnn/vscode-stylelint

Does your issue relate to non-standard syntax (e.g. SCSS, nesting, etc.)?

Nope

What did you expect to happen?

That the API adheres to the stylelintignore

What actually happened (e.g. what warnings or errors you are getting)?

All my files gets linted through the API even though they do not get linted in the CLI

Additional info

I created an issue on the vscode-stylelint repo. The author stated that there is a problem with the API. The CLI works as expected though.
https://github.com/shinnn/vscode-stylelint/issues/101#event-1219431167

@alexander-akait
Copy link
Member

alexander-akait commented Aug 24, 2017

@proProbe --ignore-path .gitignore, default ignore file have name .stylelintignore (without -files)

@proProbe proProbe changed the title Api doesnt support .gitignore? Api doesnt support .stylelintignore? Aug 24, 2017
@proProbe
Copy link
Author

@evilebottnawi Oh Im sorry, I incorrectly wrote .gitignore instead of .stylelintignore in the title. Changed that now

@proProbe proProbe changed the title Api doesnt support .stylelintignore? Api doesnt support .stylelintignore but CLI does? Aug 24, 2017
@alexander-akait
Copy link
Member

alexander-akait commented Aug 24, 2017

@proProbe for api you can use ignorePath option (https://stylelint.io/user-guide/node-api/#ignorepath and https://github.com/stylelint/stylelint/blob/master/lib/standalone.js#L59).

stylelint.lint({
  config: { ...confguration, ignorePath: '/path/to/ignore-file' },
  files: "all/my/stylesheets/*.css"
})
  .then(function(data) {
    // do things with data.output, data.errored,
    // and data.results
  })
  .catch(function(err) {
    // do things with err e.g.
    console.error(err.stack);
  });

@proProbe
Copy link
Author

@shinnn I'm kinda new to this. Any of the above suggestions that can be used?

@Arcanemagus
Copy link
Contributor

Arcanemagus commented Aug 24, 2017

@proProbe The easiest way to handle this is to ask stylelint to tell you whether the file is ignored before attempting to run a lint:

https://github.com/AtomLinter/linter-stylelint/blob/91879a162a5c2ecea0ca1f3ef481f5f96eefc8f6/lib/index.js#L164-L168

Note that in our usage we have already asked for the configuration and handled any errors that would throw, otherwise you need to handle that here.

@shinnn
Copy link
Contributor

shinnn commented Aug 24, 2017

Any of the above suggestions that can be used

No.

@proProbe
Copy link
Author

@shinnn ? So the stylelint API isnt broken? The code just need to be fixed?

@shinnn
Copy link
Contributor

shinnn commented Aug 25, 2017

So the stylelint API isnt broken?

It is broken but stylelint team doesn't seem to understand it because of your poor explanation.

The code just need to be fixed?

Yes, the code of stylelint should be fixed.

@proProbe
Copy link
Author

@shinnn I guess I dont understand the problem well enough :) maybe you could help improving the ticket so we might solve this? it seems like the atom linter works so why shouldnt the vscode?

@shinnn
Copy link
Contributor

shinnn commented Aug 25, 2017

it seems like the atom linter works

No. AtomLinter/linter-stylelint#251 (comment)

@Arcanemagus
Copy link
Contributor

Okay, there are two things going on here.


@evilebottnawi it seems that they are unintentionally correct: isPathIgnored has stopped working for .stylelintignore files. It seems the specs in linter-stylelint still only tested ignoreFiles in a config instead of the separate .stylelintignore file so I didn't see this earlier.

Back when AtomLinter/linter-stylelint#251 was closed though it seems that it was working as the test files from #1981 used a .stylelintignore, however that now fails.


@shinnn It seems stylelint has changed since the last time I looked into this, it used to be that if you explicitly called it on an ignored file it would still give you results. Ignored file processing was only done for directories if I remember correctly. The API seems to still work in this manner, where if you call lint() on a bit of code it will give you results, even if it's ignored. That's the reason isPathIgnored was added: To provide a method to check if a path would be ignored before you requested a lint.


tl;dr:
isPathIgnored is broken for .stylelintignore files now, and one could argue lint is now broken for ignored files.

@alexander-akait
Copy link
Member

@Arcanemagus thanks!

@alexander-akait alexander-akait added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule labels Aug 26, 2017
@jeddy3 jeddy3 changed the title Api doesnt support .stylelintignore but CLI does? isPathIgnored regression for .stylelintignore files Sep 2, 2017
@billybonks
Copy link
Contributor

Pre 8 when I called lint it would ignore my file if the path was matched, now it does not. this issue seems to be related to my problem. I have negotiated the issue temporarily by ignoring it manually.

billybonks/broccoli-stylelint#34

it seems a potential fix would be to add ignore.ignores(codeFileName) before calculating the absolute path.

@jeddy3 jeddy3 changed the title isPathIgnored regression for .stylelintignore files Fix isPathIgnored regression for .stylelintignore files Sep 9, 2017
@jeddy3
Copy link
Member

jeddy3 commented Sep 9, 2017

it seems a potential fix would be to add ignore.ignores(codeFileName) before calculating the absolute path.

@billybonks Thanks for digging deeper! If you find time, would you be able to contribute such a fix?

@billybonks
Copy link
Contributor

sure ill add create a pr today.

@ekfuhrmann
Copy link

Any update on the progress of this fix?

@Bigdragon13th
Copy link

gulp-stylelint and VSCode plugin .stylelintignore is also not working and I think it's related to this issue.

@thibaudcolas
Copy link
Member

thibaudcolas commented Aug 16, 2018

I've been looking into this issue, and from what I've understood so far it actually seems like the API and the CLI work the same. The difference might only be in how both are used. Would be nice if someone who is more familiar with the codebase than I am can verify my analysis.

Here is the code I think is causing this:

const ignoreFilePath = options.ignorePath || DEFAULT_IGNORE_FILENAME;
const absoluteIgnoreFilePath = path.isAbsolute(ignoreFilePath)
? ignoreFilePath
: path.resolve(process.cwd(), ignoreFilePath);
let ignoreText = "";
try {
ignoreText = fs.readFileSync(absoluteIgnoreFilePath, "utf8");

The problem is with path.resolve(process.cwd(), ignoreFilePath) – if options.ignorePath isn't used, then this will look for .stylelintignore in the current directory. I imagine most CLI usage happens from a project's root, where .stylelintignore is stored, so it might be less common of an issue. On the other hand, third-party tools relying on the API might not have their process.cwd() set to the project root where .stylelintingore is.

If options.ignorePath is used, then everything works no matter what directory you're in – so API implementers (https://github.com/shinnn/vscode-stylelint) should use that.

Alternatively, I imagine it would also be possible to change how .stylelintignore is located. For example look for it next to the current config's location (found with getConfigForFile) instead of the current directory. I'm not sure that's desirable in all scenarios though.


If anyone else wants to look into this, I've made a repository that has everything set up to try this out easily: https://github.com/thibaudcolas/stylelint-repro.

@CAYdenberg
Copy link
Contributor

Alternatively, I imagine it would also be possible to change how .stylelintignore is located. For example look for it next to the current config's location (found with getConfigForFile) instead of the current directory. I'm not sure that's desirable in all scenarios though.

I'm gonna go ahead and say this is the least bad option. In scenarios where it isn't desirable, it's possible to add the ignorePath explicitly in the config file.

We should also add some documentation around ignorePath and isPathIgnored.

I've been looking to pair remote with someone (hangouts + teletype) as a way of getting more familiar with the stylelint core so that at least someone knows how it works. If you @thibaudcolas or anyone from @stylelint/core has any interest maybe we could tackle this next week ...

@jeddy3
Copy link
Member

jeddy3 commented Aug 21, 2018

I'm gonna go ahead and say this is the least bad option.

SGTM.

I've been looking to pair remote with someone (hangouts + teletype) as a way of getting more familiar with the stylelint core so that at least someone knows how it works.

I'd be keen to understand the core more as I'm a lot more familiar with the rules than the engine itself. I suspect your JavaScript is stronger than mine @CAYdenberg, but I might know some of the historical design decisions that could help us out. I can make some time this weekend if you're around?

@CAYdenberg
Copy link
Contributor

Days change everything in the freelancers world and now I'm struggling to hit deadlines for paid work. So I'll have to take a rein check. I'll ping you in Sept on the contributors channel.

@shinnn
Copy link
Contributor

shinnn commented Aug 22, 2018

If options.ignorePath is used, then everything works no matter what directory you're in

Unfortunately no. #3590 (comment)

@jeddy3
Copy link
Member

jeddy3 commented Aug 22, 2018

Days change everything in the freelancers world

I totally understand. I freelance too :)

I'll ping you in Sept on the contributors channel.

Sounds like a plan.

@thibaudcolas
Copy link
Member

thibaudcolas commented Aug 25, 2018

@shinnn's setup in #3590 made the issue much clearer, so I've attempted a fix over at #3606.

@thibaudcolas
Copy link
Member

Looking at this issue again, I see that (at least according to the title) this is explicitly about isPathIgnored, whereas up until now I was discussing lint. The issue with lint is better covered by #3590, and addressed by #3606, but that PR doesn't change anything to isPathIgnored.

isPathIgnored doesn't support passing an ignorePath to point to a .stylelintignore, so considering it isn't documented I don't think it's fair to expect it to read ignore files from anywhere. As far as I can tell this has never been implemented, so if it was changed to do that I think this would qualify as a feature and not a bug.

What I have trouble understanding is why there are separate implementations of this ignoring logic:

  • isPathIgnored which checks the config's ignoreFiles with minimatch, and creates empty PostCSS results with an ignored flag set to true.
  • Standalone/lint's ignorer, which checks the .stylelintignore and ignorePattern (--ignore-pattern) via ignore, and filters out the ignored files from the results.

@CAYdenberg
Copy link
Contributor

This should remain open until isPathIgnored has been fixed or the documentation changed to reflect the actual behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

10 participants