-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
standard.lintFiles()
doesn't use process.cwd()
#1956
Comments
Looks like it might be relatively simple to add |
I opened the pull request standard/standard-engine#329. |
Thanks for taking the time to submit the PR. I must apologize, I'm somewhat confused on the use case you reference in the PR. It feels like you may want to investigate spawning a child process per linter instead of changing the process dir. As @wesleytodd mentioned, adjusting the cwd in flight can get hairy because so many things can rely on it. Nonetheless, your issue as described is currently possible by simply adjusting the cwd property on standard before linting the files. Which utilizing your sample code looks like the following: import fs from "node:fs/promises";
import process from "node:process";
import standard from "standard";
console.log(await fs.readFile("foo/bar.js", "utf8"));
console.log(await standard.lintFiles(["foo/bar.js"]));
process.chdir("foo");
console.log(await fs.readFile("bar.js", "utf8"));
standard.cwd = process.cwd()
console.log(await standard.lintFiles(["bar.js"])); Tested locally with node 20 and standard 17 on an intel macOS and it worked fine for me. |
I worked around the bug with this code (and the directory change works): const results = await standard.lintFiles(["bar.js"], {
cwd: process.cwd(),
}); I've opened this issue and a PR to fix the problem in Standard so that everyone can benefit from it. And also because it's strange to pass the default value; you feel like removing it because it seems useless. If you don't want to correct this problem, I think the documentation should be changed to reflect reality. |
Confirmed. I didn't understand then. The workaround (what I thought you were implying was not working properly) is what I believe should be done in this case.
I concede your point: we should not require a user to specify a default property. On the other hand, I remain convinced it's better practice to avoid defaulting to the process at function call time instead setting it when the class constructor is called. The existing code allows for this. In fact, I believe we would find the majority of users are not using I'll submit a preliminary PR, but allow time for you or others to weigh in if you feel I've missed something. Thanks again for raising this issue. |
Here's what I did
Files
package.json
index.js
foo/bar.js
Steps
npm install
node index.js
What I expected to happen
What seems to have happened
Standard doesn't use the current directory. It uses the current directory when importing
"standard"
:The text was updated successfully, but these errors were encountered: