feat(@nomiclabs/hardhat-solhint): load .solhintignore file #3741
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -84,7 +105,10 @@ function printReport(reports: any) { | |||
| subtask("hardhat-solhint:run-solhint", async (_, { config }) => { | |||
| const { processPath } = require("solhint/lib/index"); | |||
| return processPath( | |||
| join(config.paths.sources, "**", "*.sol").replace(/\\/g, "/"), | |||
| relative( | |||
There was a problem hiding this comment.
Can you add a comment explaining this and the .replace?
There was a problem hiding this comment.
Done. Let me know if it's clear enough :)
There was a problem hiding this comment.
I think this would be a bit more readable if it was done in two or three statements, with interspersed comments. Something like this:
// we need a relative path because...
const sourcesPath = relative(config.paths.root, config.paths.sources);
// we create a glob that matches all .sol files
const solFilesGlob = join(sourcesPath, "**", "*.sol");
// globs only use forward slashes, even on windows
const normalizedGlob = solFilesGlob.replace(...)Wdyt @yhuard?
There was a problem hiding this comment.
Thanks for your feedback @fvictorio I've refactored the code as you suggested.
|
I'm investigating the issue on Windows (again...) Edit: never mind, I forgot to apply |
|
Thanks for taking the time to check it @yhuard ! |
|
Nice PR! Thanks 🙌 I just left a minor comment |
fvictorio
added
status:ready
|
|
||
| // Make glob pattern relative to the current working directory | ||
| // See https://github.com/kaelzhang/node-ignore/tree/5.2.4#1-pathname-should-be-a-pathrelatived-pathname | ||
| const relativeGlob = relative(process.cwd(), solFilesGlob); |
There was a problem hiding this comment.
I think this should be relative(config.paths.root, solFilesGlob), right?
There was a problem hiding this comment.
To be honest, I'm not 100% sure. First, I was using config.paths.root but I've changed it to process.cwd() in my last commit because it made more sense to me. Solhint calls fs.readFileSync(file) where file is the relative path of the file to lint. I.e. in the context of Solhint, process.cwd() is the reference directory from which relative paths start.
In the tests, both relative(config.paths.root, solFilesGlob) and relative(process.cwd(), solFilesGlob) work.
An edge case would be if Hardhat's root folder is different from the current working directory.
For instance:
paths: {
root: "..",
sources: "solhintignore-project/contracts",
},In this case, with paths relative to process.cwd(), the tests would pass, while with paths relative to config.paths.root the tests wouldn't pass.
But this is a really unusual case. In most cases, config.paths.root work fine.
If we decide to use process.cwd(), then I'll have to make sure that all the ignore globs are relative to the current working directory as well.
If we decide to ignore this edge case (hardhat.root ≠ cwd), then I'll be happy to replace process.cwd() with config.paths.root.
I think I'm overthinking it 😅 Let me know what's best in your opinion.
There was a problem hiding this comment.
Hey @fvictorio! I've replaced process.cwd() with config.paths.root so as not to block this PR. It's highly unlikely that users will complain about the edge case I mentioned yesterday anyway.
Fixes #1665.
I'm not sure whether this is to be considered as a bugfix or a new feature.