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
Refactor the internal API #6415
Conversation
This refactoring aims to make the codebase around the internal API more consistent. Our codebase is mostly FP style, so the OOP-like code using `.bind()` seems inconsistent to me. Also, the dependency between the internal modules seems implicit through `.bind()`.
|
) => Promise<LintResult>; | ||
|
||
getConfigForFile: (searchPath?: string, filePath?: string) => Promise<CosmiconfigResult>; | ||
isPathIgnored: (s?: string) => Promise<boolean>; |
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.
[note] I believe InternalApi
and createLinter
is private and never break our ecosystem.
stylelint/types/stylelint/index.d.ts
Lines 458 to 463 in d402889
/** | |
* Internal use only. Do not use or rely on this method. It may | |
* change at any time. | |
* @internal | |
*/ | |
createLinter: (options: LinterOptions) => InternalApi; |
However, if this pull request would harm the ecosystem, I'd like to close it.
@@ -0,0 +1,26 @@ | |||
'use strict'; |
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.
[note] createLinter.test.js
is divided into two files.
const ignoreFiles = /** @type {Array<string>} */ (result.config.ignoreFiles || []).map((s) => | ||
normalizePath(s), | ||
); | ||
const ignoreFiles = [result.config.ignoreFiles || []].flat().map((s) => normalizePath(s)); |
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.
[note] The technique using .flat()
has an advantage for the type inference.
throw err; | ||
}); | ||
} | ||
} |
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.
[note] This refactoring makes the code consistent using async/await.
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.
LGTM, thank you!
Thanks for the quick review! 😄 |
Hi @jeddy3 |
@jeddy3 @ybiquitous Friendly ping |
@ricardogobbosouza @alexander-akait stylelint/types/stylelint/index.d.ts Lines 521 to 526 in b70929b
However, I'll gladly cooperate to fix the problem. |
@ybiquitous Yeah, I think we should move such things from public api to prevent such situaltion, or use |
Agree. I'll add |
None.
This refactoring aims to make the codebase around the internal API more consistent.
Our codebase is mostly FP style, so the OOP-like code using
.bind()
seems inconsistent to me.Also, the dependency between the internal modules seems implicit through
.bind()
.