sablier-labs / flow

🍃 Smart contracts of the Sablier Flow protocol.
Other
8 stars 0 forks source link

chore: bump solhint, integrate Husky and lint-staged #200

Closed smol-ninja closed 2 months ago

smol-ninja commented 2 months ago

Thanks to @PaulRBerg, we now have a robust solhint rule that can automatically fix the order of imports.

Changelog

Note

To bypass husky hook, just add -n to your commit (from git docs).

> git commit -S -n -m "your message"
PaulRBerg commented 2 months ago

I managed to make VSCode run lint:fix on save by adding this task:

{
  "version": "2.0.0",
  "tasks": [
    {
      "command": "bun run lint:fix",
      "label": "lint:fix",
      "group": {
        "isDefault": true,
        "kind": "build"
      },
      "problemMatcher": [],
      "type": "shell"
    }
  ]
}

And using this VSCode extension runonsave:

"emeraldwalk.runonsave": {
  "commands": [
    {
      "match": "\\.sol$",
      "cmd": "bun run lint:fix"
    }
  ]
},

However:

smol-ninja commented 2 months ago

Instead of vscode, how about running it through a post commit hook via Husky? So even if its slow, it would run only once automatically. The only problem is the newline. We insert newline between external and internal imports. So that still has to be taken care of manually.

PaulRBerg commented 2 months ago

Good idea with a post-commit hook!

Is that newline that much helpful? Maybe we can skip it.

smol-ninja commented 2 months ago

The fact that we need to run solhint --fix multiple times is bad because it will lead to many failed CI runs in PRs

I just checked foundry.toml also contains the double quotes rule. So if lint:fix first calls lint:sol:fix followed by forge fmt, the output will be ordered imports using double quotes.

And then add prettier:write as well as lint:fix to post-commit hooks. It should solve our problem.


Is that newline that much helpful? Maybe we can skip it.

Yes. I think the same.

smol-ninja commented 2 months ago

The fact that we need to run solhint --fix multiple times is bad because it will lead to many failed CI runs in PRs

I have integrated husky to run prettier and solhint while committing changes. In addition to that, I have also added lint-staged which only applies those changes to the staged files. Thus, solving this problem.

Screenshot 2024-08-05 at 00 06 56

We should find a way to run lint:fix while formatting files in VSCode (via saveOnFormat).

Now that it runs as a pre-commit script, we don't need this. Adding husky also means that we should never see lint failing in CI anymore.

I'd suggest to compromise on the newline in exchange for automated linting.

Note

To bypass husky hook, just add -n to your commit (from git docs).

> git commit -S -n -m "your message"
PaulRBerg commented 2 months ago

Thanks, Shub. Your changes make sense, I like them.

I pushed a commit to simplify the Lint Staged config and use the YML format instead of JSON (for consistency across our config files in general).

However, I performed some manual tests locally and I couldn't get Husky to work. Can you confirm that it works on your end? I might miss something.

smol-ninja commented 2 months ago

YML format looks better than JSON format.

I performed some manual tests locally and I couldn't get Husky to work. Can you confirm that it works on your end? I might miss something.

Are you seeing any error or you don't see any action happening at all?

I just tested it again and its working.

PaulRBerg commented 2 months ago

you don't see any action happening at all?

This. Lint Staged doesn't seem to be run, i.e., imports are not re-ordered.

smol-ninja commented 2 months ago

Nice! I also experienced the same issue on a new setup. I've fixed it (https://github.com/sablier-labs/flow/pull/200/commits/bdcab01537c320300dbfe3eeb6f6a1763c7c44f0) by adding "prepare": "husky" in package.json. This will automatically setup Husky when you run bun install. It should work for you now and if its doesn't, let me know.

Reference:

PaulRBerg commented 2 months ago

Can confirm that it worked now!

Let's hold on merging the PR until Diego follows up in that other thread.

smol-ninja commented 2 months ago

Let's hold on merging the PR until Diego follows up in that other thread.

Sounds good. But I also think, even if Diego ends up implementing double quotes, this integration would still be useful.

The following configuration would remain unchanged. Because we need both solhint and forge fmt, only the order of running these commands will stop to matter.

"*.{json,md,svg,yml}": "prettier --write"
"*.sol":
  - "bun solhint --fix --noPrompt"
  - "forge fmt"