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 some path / glob problems #4867

Merged
merged 7 commits into from Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/__tests__/fixtures/globs/[digit]/not-digits/styles.css
@@ -0,0 +1 @@
a {}
1 change: 1 addition & 0 deletions lib/__tests__/fixtures/globs/extglob!(s)/styles.css
@@ -0,0 +1 @@
a {}
1 change: 1 addition & 0 deletions lib/__tests__/fixtures/globs/glob+chars/glob-plus.css
@@ -0,0 +1 @@
a {}
1 change: 1 addition & 0 deletions lib/__tests__/fixtures/globs/glob-contains-plus/styles.css
@@ -0,0 +1 @@
a {}
1 change: 1 addition & 0 deletions lib/__tests__/fixtures/globs/got!negate/negate/styles.css
@@ -0,0 +1 @@
a {}
@@ -0,0 +1 @@
a {}
@@ -0,0 +1 @@
a {}
@@ -0,0 +1 @@
a {}
1 change: 1 addition & 0 deletions lib/__tests__/fixtures/globs/with spaces/styles.css
@@ -0,0 +1 @@
a {}
193 changes: 193 additions & 0 deletions lib/__tests__/standalone-globs.test.js
@@ -0,0 +1,193 @@
'use strict';

/* eslint-disable node/no-extraneous-require */

const describe = require('@jest/globals').describe;
const expect = require('@jest/globals').expect;
const it = require('@jest/globals').it;

/* eslint-enable */

const path = require('path');
const replaceBackslashes = require('../testUtils/replaceBackslashes');
const standalone = require('../standalone');

const fixturesPath = replaceBackslashes(path.join(__dirname, 'fixtures', 'globs'));

// Tests for https://github.com/stylelint/stylelint/issues/4521

describe('standalone globbing', () => {
describe('paths with special characters', () => {
// ref https://github.com/micromatch/micromatch#matching-features
const fixtureDirs = [
`[digit]/not-digits`,
`with spaces`,
`extglob!(s)`,
`got!negate/negate`,
// `extglob+(s)`, // Note: +'s cause errors. Ignoring until it becomes a problem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, what's the error here? Should we document that there's a problem with the + character?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I should have made a note of it. The + seemed to be escaped differently for this case, causing the expected match to fail.

I can't remember the specific details, but I think I tracked it down to somewhere in one of globby's dependencies. I thought I'd read an issue that described it, but now I can't find it 🤷

It seemed like enough of an edge-case that we can skip it for now, and investigate later if it becomes a problem.

];

// https://github.com/stylelint/stylelint/issues/4193
it.each(fixtureDirs)(`static path contains "%s"`, async (fixtureDir) => {
const cssPath = `${fixturesPath}/${fixtureDir}/styles.css`;

const { results } = await standalone({
files: cssPath,
config: { rules: { 'block-no-empty': true } },
});

expect(results).toHaveLength(1);
expect(results[0].errored).toEqual(true);
expect(results[0].warnings[0]).toEqual(
expect.objectContaining({
rule: 'block-no-empty',
severity: 'error',
}),
);
});
});

// https://github.com/stylelint/stylelint/issues/4211
it('glob has no + character, matched path does', async () => {
const files = `${fixturesPath}/**/glob-plus.css`; // file is in dir 'glob+chars'

const { results } = await standalone({
files,
config: { rules: { 'block-no-empty': true } },
});

expect(results).toHaveLength(1);
expect(results[0].errored).toEqual(true);
expect(results[0].warnings[0]).toEqual(
expect.objectContaining({
rule: 'block-no-empty',
severity: 'error',
}),
);
});

// https://github.com/stylelint/stylelint/issues/4211
it('glob contains + character, matched path does not', async () => {
const files = `${fixturesPath}/+(g)lob-contains-plus/*.css`;

const { results } = await standalone({
files,
config: { rules: { 'block-no-empty': true } },
});

expect(results).toHaveLength(1);
expect(results[0].errored).toEqual(true);
expect(results[0].warnings[0]).toEqual(
expect.objectContaining({
rule: 'block-no-empty',
severity: 'error',
}),
);
});

// https://github.com/stylelint/stylelint/issues/3272
// should ignore 'negated-globs/ignore/styles.css'
it('negated glob patterns', async () => {
const files = [
`${fixturesPath}/negated-globs/**/*.css`,
`!${fixturesPath}/negated-globs/ignore/**/*.css`,
];

const { results } = await standalone({
files,
config: { rules: { 'block-no-empty': true } },
});

// ensure that the only result is from the unignored file
expect(results[0].source).toEqual(expect.stringContaining('lint-this-file.css'));

expect(results).toHaveLength(1);
expect(results[0].errored).toEqual(true);
expect(results[0].warnings[0]).toEqual(
expect.objectContaining({
rule: 'block-no-empty',
severity: 'error',
}),
);
});

describe('mixed globs and paths with special chars', () => {
it('manual escaping', async () => {
const cssGlob = `${fixturesPath}/got\\[braces\\] and \\(spaces\\)/*.+(s|c)ss`;

const { results } = await standalone({
files: cssGlob,
config: {
rules: {
'block-no-empty': true,
},
},
});

expect(results).toHaveLength(1);
expect(results[0].errored).toEqual(true);
expect(results[0].warnings[0]).toEqual(
expect.objectContaining({
rule: 'block-no-empty',
severity: 'error',
}),
);
});

it('setting "cwd" in globbyOptions', async () => {
const cssGlob = `*.+(s|c)ss`;

const { results } = await standalone({
files: cssGlob,
config: {
rules: {
'block-no-empty': true,
},
},
globbyOptions: {
cwd: `${fixturesPath}/got[braces] and (spaces)/`,
},
});

expect(results).toHaveLength(1);
expect(results[0].errored).toEqual(true);
expect(results[0].warnings[0]).toEqual(
expect.objectContaining({
rule: 'block-no-empty',
severity: 'error',
}),
);
});

/* eslint-disable jest/no-commented-out-tests -- Failing case for reference. Documents behaviour that doesn't work. */

// Note: This fails because there's no way to tell which parts of the glob are literal characters, and which are special globbing characters.
//
// 'got[braces] and (spaces)' is a literal directory path. `*.+(s|c)ss` is a glob.

// https://github.com/stylelint/stylelint/issues/4855
// it('glob and matched path contain different special chars, complex example', async () => {
// const cssGlob = `${fixturesPath}/got[braces] and (spaces)/*.+(s|c)ss`;

// const { results } = await standalone({
// files: cssGlob,
// config: {
// rules: {
// 'block-no-empty': true,
// },
// },
// });

// expect(results).toHaveLength(1);
// expect(results[0].errored).toEqual(true);
// expect(results[0].warnings[0]).toEqual(
// expect.objectContaining({
// rule: 'block-no-empty',
// severity: 'error',
// }),
// );
// });

/* eslint-enable */
});
});
15 changes: 15 additions & 0 deletions lib/standalone.js
Expand Up @@ -4,6 +4,7 @@ const _ = require('lodash');
const createStylelint = require('./createStylelint');
const createStylelintResult = require('./createStylelintResult');
const debug = require('debug')('stylelint:standalone');
const fastGlob = require('fast-glob');
const FileCache = require('./utils/FileCache');
const filterFilePaths = require('./utils/filterFilePaths');
const formatters = require('./formatters');
Expand Down Expand Up @@ -170,6 +171,20 @@ module.exports = function (options) {
fileList = [fileList];
}

fileList = fileList.map((entry) => {
if (globby.hasMagic(entry)) {
const cwd = _.get(globbyOptions, 'cwd', process.cwd());
const absolutePath = !path.isAbsolute(entry) ? path.join(cwd, entry) : path.normalize(entry);

if (fs.existsSync(absolutePath)) {
// This glob-like path points to a file. Return an escaped path to avoid globbing
return fastGlob.escapePath(entry);
}
}

return entry;
});

if (!options.disableDefaultIgnores) {
fileList = fileList.concat(ALWAYS_IGNORED_GLOBS.map((glob) => `!${glob}`));
}
Expand Down
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -118,6 +118,7 @@
"cosmiconfig": "^7.0.0",
"debug": "^4.1.1",
"execall": "^2.0.0",
"fast-glob": "^3.2.4",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already a dependency of globby, so it's not really a new dependency here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just note - prettier and many other tools remove globby and use fast-glob directly, maybe we can do same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting! That could be a good follow-up PR.

It would need to be a breaking change, I think. As globbyOptions is a config option for stylelint. globby has a couple of options that aren't supported in fast-glob. Though I'd guess few people are using any of the globby-only option..

"fastest-levenshtein": "^1.0.9",
"file-entry-cache": "^5.0.1",
"get-stdin": "^8.0.0",
Expand Down