foundry icon indicating copy to clipboard operation
foundry copied to clipboard

feat(lint): refactor LinterConfig and add 3 new linting rules

Open milosdjurica opened this issue 1 month ago • 7 comments

  • Added a new linting rule that checks for multiple contract-like items (contracts, abstract contracts, interfaces and libraries) per .sol file .
  • Refactored LinterConfig
  • Added 2 new linting rules that checks for interface names and interface file names

Motivation

Enforce best practice.

Solution

I have some doubts regarding this, would like to hear your opinion.

  1. Should the rule skip interfaces and libraries and count only contracts?
  2. Should the rule show linting note for every contract-like item in the file, or only one time per file is enough?

My solution count interfaces and libraries too, and it shows linting note only once per file. Let me know if you want something to be changed :)

PR Checklist

  • [x] Added Tests
  • [ ] Added Documentation
  • [ ] Breaking changes

Keep up the good work guys, cheers!

milosdjurica avatar Nov 16 '25 14:11 milosdjurica

@milosdjurica first of all thanks for the PR and the initiative, we love new contributions to forge's linter!

i think the idea overall makes sense, however imo we can improve it.

  1. i'd like each contract to be flagged, not just the 2nd one
  2. imo it would make sense to add a custom config for this lint, so that users can decide if they want to allow interfaces and/or libs in the same file. To do so, you'd want to modify the LinterConfig to have a dedicated config for this lint. If we do that, i think we then want to create a new intermediate struct that group all lint configs (i.e. the pre-existing mixed_case_exceptions + your newly introduced config)

lmk if this seems reasonable and if u have any doubts regarding impl

0xrusowsky avatar Nov 17 '25 15:11 0xrusowsky

@milosdjurica first of all thanks for the PR and the initiative, we love new contributions to forge's linter!

i think the idea overall makes sense, however imo we can improve it.

  1. i'd like each contract to be flagged, not just the 2nd one
  2. imo it would make sense to add a custom config for this lint, so that users can decide if they want to allow interfaces and/or libs in the same file. To do so, you'd want to modify the LinterConfig to have a dedicated config for this lint. If we do that, i think we then want to create a new intermediate struct that group all lint configs (i.e. the pre-existing mixed_case_exceptions + your newly introduced config)

lmk if this seems reasonable and if u have any doubts regarding impl

Sounds great, I like it! Will look to implement it ASAP :) Thanks for the response!

milosdjurica avatar Nov 17 '25 19:11 milosdjurica

@milosdjurica lmk when the PR is ready for re-review!

0xrusowsky avatar Nov 24 '25 22:11 0xrusowsky

Hi @0xrusowsky , it should be good now 👍🏻

milosdjurica avatar Nov 24 '25 23:11 milosdjurica

great, tysm! will review it today at some point

0xrusowsky avatar Nov 25 '25 06:11 0xrusowsky

@milosdjurica btw, i haven't tested the my code compiles, those are just suggestions, so you may need to tweak them a bit, but u get the gist of the requested simplifications

0xrusowsky avatar Nov 30 '25 09:11 0xrusowsky

Hi @0xrusowsky, thanks for suggestions - it's much cleaner now. I have applied them with some small changes, lmk if this is okay now

milosdjurica avatar Dec 02 '25 19:12 milosdjurica

Hi guys, @0xrusowsky @grandizzy - just a quick ping in case you missed the latest changes 👀

milosdjurica avatar Dec 12 '25 11:12 milosdjurica

@grandizzy good to merge?

Yep, let's send it 👍

grandizzy avatar Dec 13 '25 07:12 grandizzy