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
Throw error on non-existent files unless allow-empty-input is enabled #3965
Throw error on non-existent files unless allow-empty-input is enabled #3965
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bilie Thanks for doing this. It's looking good.
I have a query about the wording of the error message.
This is a breaking change, as should be within a major release.
Major releases can be tricky as most plugins with peer dependencies have to release new versions. @stylelint/core Do we release regardless?
lib/__tests__/ignore.test.js
Outdated
@@ -191,8 +191,11 @@ describe("extending config with ignoreFiles glob ignoring one by negation", () = | |||
}); | |||
|
|||
describe("specified `ignorePath` file ignoring one file", () => { | |||
let results; | |||
const noFilesErrorMessage = new Error( | |||
"The specified `files` glob returns no results" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we adopt the ESLint approach of?:
No files matching the pattern "${pattern}" were found.
This feels a little more explicit as we're stating the glob pattern in the error message, rather than just referring to it.
With talk of a major release in #4000, I think we can get this in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
I just have two small change requests.
lib/standalone.js
Outdated
@@ -14,6 +14,7 @@ const globby /*: Function*/ = require("globby"); | |||
const hash = require("./utils/hash"); | |||
const ignore = require("ignore"); | |||
const needlessDisables /*: Function*/ = require("./needlessDisables"); | |||
const NoFilesFoundError = require("./testUtils/noFilesFoundError"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think noFilesFoundError
should be in lib/utils
directory, because it's a part of application.
lib/__tests__/standalone.test.js
Outdated
config: configBlockNoEmpty | ||
}) | ||
.then(() => { | ||
throw new Error("should not have succeeded"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious why to throw an error here? Isn't stylelint should throw an error and catch
will catch it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I removed this, it was left over from testing
@hudochenkov Good catches! |
@Bilie Do you think you'll have time to look into @hudochenkov suggestions before the weekend? We're planning to release |
Co-Authored-By: Bilie <bilianavaleva@gmail.com>
Hi, thank you for the feedback! I have updated the PR, let me know if you spot anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing feedback!
One last thing. I believe all throw new Error("should not have succeeded");
in ignore.test.js
are also unnecessary for tests.
Look awesome! Thanks, @Bilie! |
|
- 避免 packing.stylelint.options.files 匹配不到文件时报错 - stylelint@>=10.0 - @see stylelint/stylelint#3965
Closes #3880