defi-wonderland / natspec-smells

👃Automatically identify missing or incomplete natspec
MIT License
90 stars 7 forks source link

moar feedback #43

Open devtooligan opened 7 months ago

devtooligan commented 7 months ago

Tried using this for the second time on another project and this time I could not get it working at first. I got confused by the "npx @defi-wonderland..." example. Then I remembered to just yarn install it and use it locally with a config file and that worked 👌🏽

But I still could not figure out how to have it search through nested folders. Here is my config.js:

module.exports = {
    include: "contracts/*.sol",
    enforceInheritdoc: "false",
};

Which works, but it ignores any folders nested in contracts. This works for the subfolders:

module.exports = {
    include: "contracts/*/*.sol",
    enforceInheritdoc: "false",
};

But now it does not include the contracts in contracts/.

I tried the syntax shown on the readme and other combinations but it did not work:

By the way one of the examples in the readme uses two asterisks ** but using that elicits a weird error so I think that's a mistake.

So anyways, I ran it twice with two different configs in order to get it to process all the files.

The output is still challenging because it groups Variables and Interfaces together. I am looking forward to the ability to exclude variables, internal/private functions, and Interfaces. Alternatively (or additionally) maybe it would be helpful if the output itself indicated what the thing is like variable, event, function etc

Here are the problems I was having with "npx @defi-wonderland...":

There might be a typo in the docs as it says:

Remember to put quotes around the glob strings when using the include and exclude options.

But the example given, does not include quotes around src after the include and the quote-enclosed strings after --exclude do not appear to be correct syntax

npx @defi-wonderland/natspec-smells --include src --exclude "src/*/.sol" "(test|scripts)/*/.sol"

Basically, no matter what syntax I tried, I

gas1cent commented 7 months ago

Thank you for the feedback!

By the way one of the examples in the readme uses two asterisks ** but using that elicits a weird error so I think that's a mistake.

Could you show the weird error please? The double asterisk is exactly how both include and exclude should be configured to reach sub-directories.

But the example given, does not include quotes around src after the include and the quote-enclosed strings after --exclude do not appear to be correct syntax

Good catch, fixing.

Basically, no matter what syntax I tried, I

Haha, that's one hell of a cliffhanger!

I am looking forward to the ability to exclude variables, internal/private functions, and Interfaces.

This is going to be shipped in 1-2 weeks. We're tweaking the config to let you choose which tags are used for example in internal functions and completely disable the checks for any types of nodes.

devtooligan commented 7 months ago

When my config is:

module.exports = {
    include: "contracts/**/*.sol",
    exclude: "I*.sol",
    enforceInheritdoc: "false",
};

I get:

yarn natspec-smells
yarn run v1.22.21
$ natspec-smells
error Command failed with signal "SIGBUS".
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
y

but when my config is:

module.exports = {
    include: "contracts/*/*.sol",
    exclude: "I*.sol",
    enforceInheritdoc: "false",
};

It works and behaves as expected.

Basically, no matter what syntax I tried, I

I was gonna finish that sentence, and then I got high

gas1cent commented 7 months ago

error Command failed with signal "SIGBUS"

Oh, I actually have a theory and possibly a solution for this error, but I haven't been able to reproduce it in a while. Do you mind sharing the repo for me to verify the idea?

I was gonna finish that sentence, and then I got high

Glad it wasn't a snipe

devtooligan commented 7 months ago

Do you mind sharing the repo for me to verify the idea?

sorry it's private