Optional list of imports on acyclic_siblings rendering

[K4liber]

Using the acyclic_siblings contract I get the following log:

----------------
Broken contracts
----------------

AcyclicDependencies
-------------------

No cycles are allowed in definit_db.data.field.mathematics.definitions.
It could be made acyclic by removing 2 dependencies:

- .number -> .fundamental (1 import)
- .problem -> .algorithm (3 imports)

How about the additional contract option called "list_imports" with a default value "false"? If "true", just below each package dependency, it will print the imports responsible for the dependency. In this case:

- .number -> .fundamental (1 import)
  definit_db.data.field.mathematics.definitions.number.integer -> definit_db.data.field.mathematics.definitions.fundamental.object
- .problem -> .algorithm (2 imports)
  definit_db.data.field.mathematics.definitions.problem.optimization -> definit_db.data.field.mathematics.definitions.algorithm.objective
  definit_db.data.field.mathematics.definitions.problem.linear_programming -> definit_db.data.field.mathematics.definitions.algorithm.objective

[seddonym]

Thanks for the idea. I agree, it would be good to improve this behaviour.

My immediate reaction is that display/reporting concerns like this should probably be controlled by cli options rather than on the contract. That would involve rethinking the contract API, e.g. so there are different levels of verbosity, or there are custom arguments that make their way to the render_broken_contract method.

Alternatively, we keep the linter as-is and improve the tooling around understanding the cycles - possibly in https://impulse-cli.readthedocs.io/.

What do you think?

[K4liber]

How about utilizing the verbose flag not only in the check method, but also in the render_broken_contract method? And then using the flag inside the method AcyclicSiblingsContract.render_broken_contract to decide wherever to print the imports or not.

[seddonym]

How about utilizing the verbose flag not only in the check method, but also in the render_broken_contract method?

That could work. But it is a potential breaking change (every custom contract type would start erroring if we're not careful). Maybe we could use inspect.signature to figure out whether to pass verbose in. (In retrospect I should have made the API forward-compatible with **kwargs!)

I do wonder though if the current model of verbosity (a boolean) isn't quite right. In forbidden contracts, for example, the verbose output is very verbose and can actually slow down the check considerably. Maybe it's time to look at how we handle that more generally.

With that in mind, it might be better to start looking into additional tooling for investigating the import graph, built in to Import Linter. One idea might be to have an interactive mode that you can enter for each contract where you'd be able to drill down into more detail. That's obviously a lot more work though than just putting a list_imports field on the contract!

I am not sure of the best way forward at this point, would like to reflect further but keen to hear others' thoughts.

[K4liber]

Thanks for the comprehensive answer. When it comes to me, I just wanted the acyclic contract to be more verbose, not even thinking about handling the issue in a more holistic way.

How about sneaking the verbose flag inside check through metadata and using it in the render, too much hacky?

[seddonym]

How about sneaking the verbose flag inside check through metadata and using it in the render, too much hacky?

I hadn't thought of that. It's a nice idea, though the verbose flag can undermine the performance of other contracts in the same run, so it's not without downsides.

I have actually now begun work on an interactive mode for Import Linter so possibly we should wait until there is more progress there. In the meantime, you could make a custom contract type that extends AcyclicSiblingsContract and outputs the data in the detail you need. How does that sound?

[K4liber]

Yes, thanks, I will try to create a custom one :)