Correct exit status when explicitly excluded deps are unused

[sanmai-NL]

Why is the change needed?

The exit status is incorrect when excluded deps are detected.

What was done in this PR?

Are there any concerns, side-effects, additional notes?

Checklist, when applicable

• Added test(s)
• Updated README.md
• Is this a backwards-compatibility breaking change?
• Resolves: #issue-number-here

[fredrikaverpil]

Hi and thanks for this @sanmai-NL!

I would just like to have a test added for this before merging this change. I can look into this later tonight at earliest.

[fredrikaverpil]

Hi again, can you please explain what you mean here?

I just added this test, which means to fail (exit code 1) if you:

• have excluded a dependency with --exclude-deps
• and said dependency is not defined in your deps file (e.g. pyproject.toml)
• and said dependency is not installed in the virtual environment

This is basically like running the following command in this repo:

$ creosote --exclude-deps nonexistentdependency
Found dependencies in pyproject.toml: distlib, dotty-dict, loguru, pip-requirements-parser, toml
Excluded dependencies not found in virtual environment: nonexistentdependency
Oh no, bloated venv! 🤢 🪣
Unused dependencies found: nonexistentdependency

echo $?
1

As you can see, this test fails this PR, which makes me wonder what behavior you are expecting.

[fredrikaverpil]

@sanmai-NL I'm going to close this. Please feel free to reopen if you think I'm wrong to close this.

[sanmai-NL]

I‘m speaking of product as your user's package, rather than project (frequent misnomer in the OSS world).

@fredrikaverpil
Why should this test case cause a failure exit status? That case produces a fault, perhaps, since Creosote‘s config then doesn‘t match the package at hand (deps are excluded explicitly, that aren‘t deps). However, the functionality of Creosote is to find unused deps, not [to find discrepancies between used deps and the resolved product‘s dep config including Creosote‘s config]*. You could either exit with 0 and log a warning about this discrepancy, or, if your broaden the functionality of Creosote*, exit with a specific failure-indicating exit status, like 2 instead of 1, reserving 1 for when a legit unused dependency is found. I would want to override that behavior since I don't consider the discrepancy a defect. I want to be able to globally use a Creosote config across our products, even if some don't use some problematic dep (problematic as in, with implicit use-relation, by design).

Furthermore, whether a package is installed in the venv, should be left out of consideration (i.e., not be a criterion in your algorithm, written in your last post). What matters is what the product specificies in terms of deps. Only when checking against de facto deps of the product iff globally installed packages and iff the product specific venv is disabled, you would perhaps want to assume the dep is satisfied (with a warning). Again, this is not related to functionality of Creosote as unused dep finder, only for *.

[fredrikaverpil]

Okay, I see. I think you have a point, so I am reopening this issue.

I'm not sure I see how this can be beneficial in your case, but I trust you have a use case where you need this. To me, this would be a misconfiguration of creosote, but like you say not necessarily a defect that warrants a non-zero exit code and a failing CI check. Most often with CI, you don't look at the logs every time you run the checks. If your checks passes, you assume everything was fine. By failing when there is a misconfiguration, this will bring attention to the fact that you likely forgot to update your --exclude-deps argument value(s), and which I would appreciate.

I don't think I want to introduce different error codes here. I would then rather introduce a commandline option which can toggle this behavior. Since I personally would like a failure by default, I'm now contemplating whether to add in a CLI argument that can ignore excluded-but-unused dependencies. This means you would have to specify this command line argument in your setup, so to get the behavior you desire. Do you think this is a good tradeoff?

[sanmai-NL]

My use case is that we have default templates for Python products and want to exclude known problematic deps from our default Creosote config, whether they are specified as deps in specific Python products or not.

[sanmai-NL]

In any case, the message 'Unused dependencies found' is incorrect if an excluded dep is indeed unused, but also unspecified.

[fredrikaverpil]

Let's continue this in #161