Check contracts from multiple files on a single graph

[seddonym]

Currently, Import Linter expects a single contract file per graph build: each .ini/.toml file defines top level configuration which will determine how the graph is built, which the contracts then run on.

It would be helpful to decouple this so that contracts can be split across multiple files without needing the overhead of rebuild the graph each time. For larger codebases this would help with code ownership of sets of contracts.

Possible API:

lint-imports --config=main.ini,extra_one.ini,extra_two.toml

Or:

lint-imports --config=main.ini --extra-config=extra_one.ini --extra-config=extra_two.toml

Might also be worth supporting globs e.g.

lint-imports --config=main.ini --extra-configs=path/*.ini

(Or at least designing the API so that it could support that in future.)

[PaulGrimshaw]

+1 for supporting the globs idea. This would save multiple developers needing to dig up the different places that the import linter is called to update each time a new file is added. Also keeps the commands a bit more concise.

Feel the api would be cleaner if --config just accepted a glob? e.g. lint-imports --config=path/*.ini

[Peter554]

One possible alternative - allow including one config from another:

# main.ini

[importlinter]
#  Paths relative to main.ini
#  Maybe accept globs here?
include = ["foo.ini", "bar.ini"] 

Not sure that this is better, just putting the idea out there.

One way that this might be nicer is that it gives an API to run just a subset of the contracts, from only one of the included configs. E.g. rough idea:

lint-imports --config main.ini --contracts-from foo.ini

[seddonym]

Another option: we start accepting the config files as positional arguments to lint-imports. That would allow us to leverage the user's shell for glob expansion (in the same way as ls does).

For example:

lint-imports main.ini foo.ini

And:

lint-imports main.ini extra_contracts/*.ini

We'd need to keep --config for backwards compatibility though.

[seddonym]

How to handle top-level configuration

The main design consideration with supplying more than one file is that there might be more than one top level configuration.

I think we have these options:

1. Use the top level configuration of the first supplied file. (We'd need to make sure there is a way to force this to a particular file if we are using glob expansion - probably just explicitly list that file first.)
2. Error if there is more than one top level configuration.
3. Allow multiple top level configurations providing they are identical. Otherwise, error.
4. Option 2, but warn instead of erroring (and use the first one).
5. Option 3, but warn instead (and use the first one).
6. Build separate graphs for each top level configuration - just share them as an optimization step.

[seddonym]

How to handle contracts with identical ids

The linter can currently be run on specific contracts by passing --contract. https://import-linter.readthedocs.io/en/stable/usage.html#running-the-linter. This uses the id, which will always be present in .ini files, due to the syntax (unliketoml files, where it needs to be provided explicitly).

So, the question is: what should be the behaviour if multiple contracts use the same id? While it's already a possibility with toml, it now becomes possible for .ini` files. Options:

1. Error if the same id is found across files, regardless of whether --contract is passed.
2. Error if the same id is found across files, if --contract is passed and the id it provides is ambiguous.
3. Use the id from the first passed file.
4. Use the first id, but warn the user that it's ambiguous.

There's then the question of what course of action we recommend to the user. Options:

1. Edit the files so that the ids are unique.
2. Support an optional way of disambiguating from the command line based on what file it's in, e.g. --contract=path/to/config.ini:my-contract-id.
3. Transform the ids into unique identifiers if there is a collision. E.g. colliding ids could be appended with a hash of the filename (--contract=my-contract-id-a3b232) or a deterministic order (--contract=my-contract-id-2).

[seddonym]

On the above comments, my current thinking is that we should maximise flexibility - so someone should be able to combine runs across multiple files without needing to edit the files, just by tweaking the cli invocation. I think the simplest experience for the users would be to just to build separate graphs for each file (and as an optimization step, don't bother rebuilding the graph if it's going to be identical).

Regarding contract ids:

• If an ambiguous contract id is passed on the command line, we should error, providing them with options to disambiguate using the filename, as per option 2 above. (By default the filename should be relative to the current working directory.)
• In the specific case of a single toml file containing two contracts with the same id, that should error in a different way: we should force them to edit the file. (This should error regardless of whether --contract is passed, as that file is invalid.)

I also think the config files should be passed as positional arguments as per my earlier comment. For simplicity, I don't think we should do any glob-expansion within Import Linter, let's just leave it up to the user's shell.

So an invocation might look something like:

lint-imports main.ini extra_contracts/*.ini --contract=my-contract

Multiple contracts found with the id `my-contract`. Try including the filename, e.g.

--contract=main.ini:my-contract
--contract=extra_contracts/two.ini:my-contract

Happy to hear other opinions!