use solhint-community

[juanpcapurro]

This PR

• switches from solhint to solhint-community, to just released version 3.6.0,
  which includes fixes for several false positives previously reported
• has no-unused-imports enabled by default since the config extends the
  recommended ruleset
• explicitly enables named-parameters-function and fixes a few instances of
  invocations with 4+ positional arguments

[PaulRBerg]

Thanks for your PR, @juanpcapurro.

Unfortunately, I don't think that the named-parameters-function rule is working as expected.

I undid all changes in "src", set the rule to "off", and I ran pnpm lint - there was no error caught by the lint command.

Also, on a separate note: we would prefer not to make any changes in src in this PR, because we've just launched Sablier V2 and we'd prefer the source code to remain identical for a while. Opened an issue for tracking: #150

[juanpcapurro]

Unfortunately, I don't think that the named-parameters-function rule is working as expected.

I'm not sure I follow. I separated the commit fixing the issues reported by
named-parameters-function and the addition of the rule to make it clearer to
illustrate:

on commit e184c13 add named-parameters-function rule, the rule is added but
no fix for its reported errors is yet implemented:

[N] capu ~/s/v2-periphery ((e184c137))> pnpm lint
...
> forge fmt --check && pnpm solhint "{script,src,test}/**/*.sol"
script/DeployProtocol.s.sol
  38:25  error  Call to function with arity > 3 is using positional arguments. Use named arguments instead  named-parameters-function
...
test/utils/Defaults.sol
  76:26  error  Call to function with arity > 3 is using positional arguments. Use named arguments instead  named-parameters-function
✖ 22 problems (22 errors, 0 warnings)
 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Command failed with exit code 1.
[I] capu ~/s/v2-periphery ((e184c137)) [1]>

on commit 49d72b3 fix named-parameters-function solhint reports, the fixes
are implemented, and no errors are reported:

[I] capu ~/s/v2-periphery (use-solhint-community)> pnpm lint
> @sablier/[email protected] lint /home/capu/src/v2-periphery
> pnpm lint:sol && pnpm prettier:check
> @sablier/[email protected] lint:sol /home/capu/src/v2-periphery
> forge fmt --check && pnpm solhint "{script,src,test}/**/*.sol"
> @sablier/[email protected] prettier:check /home/capu/src/v2-periphery
> prettier --check "**/*.{json,md,yml}"
Checking formatting...
All matched files use Prettier code style!

I undid all changes in "src", set the rule to "off", and I ran pnpm lint - there was no error caught by the lint command.

I think that's to be expected. If you disable the rule, then you won't get
reports for code that's not compliant with it.

Or do you mean the rule interferes with the reports for other rules? Currently I
can't reproduce that:

[I] capu ~/s/v2-periphery (use-solhint-community)> git checkout 49d72b3 # tip of this branch
[N] capu ~/s/v2-periphery ((49d72b34))> git checkout d9c6943 src # current 'main' tip
Updated 3 paths from b32d248
[I] capu ~/s/v2-periphery ((49d72b34))> nvim .solhint.json
[I] capu ~/s/v2-periphery ((49d72b34))> git diff .solhint.json
diff --git a/.solhint.json b/.solhint.json
index 19e2627..b85e9e8 100644
--- a/.solhint.json
+++ b/.solhint.json
@@ -10,7 +10,7 @@
     "func-visibility": ["error", { "ignoreConstructors": true }],
     "max-line-length": ["error", 123],
     "named-parameters-mapping": "warn",
-    "named-parameters-function": "error",
+    "named-parameters-function": "off",
     "no-empty-blocks": "off",
     "not-rely-on-time": "off",
     "var-name-mixedcase": "off"
[I] capu ~/s/v2-periphery ((49d72b34))> git status
HEAD detached at 49d72b3
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        modified:   src/SablierV2ProxyTarget.sol
        modified:   src/types/Permit2.sol
        modified:   src/types/Proxy.sol

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
  (commit or discard the untracked or modified content in submodules)
        modified:   .solhint.json
        modified:   lib/prb-proxy (new commits)
        modified:   lib/v2-core (new commits, untracked content)
[I] capu ~/s/v2-periphery ((49d72b34))> pnpm lint
...
> forge fmt --check && pnpm solhint "{script,src,test}/**/*.sol"
src/types/Permit2.sol
  5:10  warning  imported name IPermit2 is not used  no-unused-import

src/types/Proxy.sol
   9:10  warning  imported name IPRBProxy is not used          no-unused-import
  10:10  warning  imported name IPRBProxyPlugin is not used    no-unused-import
  11:10  warning  imported name IPRBProxyRegistry is not used  no-unused-import

✖ 4 problems (0 errors, 4 warnings)
...

Also, on a separate note: we would prefer not to make any changes in src in this PR, because we've just launched Sablier V2 and we'd prefer the source code to remain identical for a while. Opened an issue for tracking: #150

Understandable. Is there any action I should take? should I mark this PR as a draft?

[PaulRBerg]

Thanks for the detailed response!

I think that's to be expected. If you disable the rule, then you won't get
reports for code that's not compliant with it.

Yes, you are right. Apologies, there was a misconception on my part in my previous reply. The named-parameters-function rules works as expected.

Is there any action I should take? should I mark this PR as a draft?

Undo all changes in src and do not activate the named-parameters-function rule. Happy to accept the other changes.

[juanpcapurro]

Undo all changes in src and do not activate the named-parameters-function rule. Happy to accept the other changes.

done!

I took the liberty of setting solhint-community to the release candidate for the next minor version, but I can set it to the last stable version if you prefer.

I decided to work with release candidates since I don't want issues like the ones found in no-unused-import to reach users of stable versions, but I of course need the collaboration of a few projects so the release candidates get some real-world exposure to find said issues 🙃

[PaulRBerg]

Thank you, @juanpcapurro!

I decided to work with release candidates

Nice write-up. We're glad to support you in your development by being early adopters.

It makes sense to use a release candidate versioning system!

setting solhint-community to the release candidate for the next minor version, but I can set it to the last stable version if you prefer.

Using rc00 works!