Detect if imports are done within try/except blocks

[gforcada]

It is quite common, for backwards compatibility code, to have fallback imports, i.e.

try:
    from new.shiny import stuff
except ImportError:
    from old.hazard import stuff

It would be good if z3c.dependencychecker does not suggest that both new.shiny and old.hazard should be declared as dependencies.

Not sure exactly what it should suggest though?

• ❓ none
• ❓ only one, which one?
• ❌ both

None feels wrong, as either one or the other should be needed to have stuff working down the module.

Both feels way wrong, because having two imports providing the same from different packages is probably what you don't want.

Thinking a bit more, maybe mentioning them both and letting the user decide about it (i.e. not reporting it as broken) might be better, as the example above is simple, but it can be more complex, so a default option might not be desirable.

Opinions? 🍀

[reinout]

At least the new one will be in the dependencies. So it is OK to report if it is missing. And if it is actually neatly in the dependencies, it won't get reported.

The old one might be an old stdlib import for an old location. Or an "old-library if python < 2.6"-type dependency.
You could simply put that into the ignore-packages list, I guess? Isn't that the easiest solution?

If so, it would be a good idea to mention it in the readme.

[gforcada]

Well, the question is that, say you have the snippet from the first message buried somewhere in a module in a distribution that you don't know by heart, then you run z3c.dependencychecker and you get:

Missing requirements
====================
     new.shiny.stuff
     old.hazard.stuff

Without more context, and potentially with plenty of other imports around, you might simply think that both are needed as they are doing completely different things...

Instead if it was reported like this:

try/except imports
==================
     new.shiny.stuff
     old.hazard.stuff

Then it already highlights to you that before adding them on setup_requires or in ignore-packages you better open that module and check where that is actually being used.

That's what's happening to me these last few days as I'm configuring plenty of random plone distributions that, by any means, I know their ins and outs 😅

[mauritsvanrees]

Example is this code in plone.outputfilters:

try:
    from zope.component.hooks import getSite
except ImportError:
    from zope.app.component.hooks import getSite

In a current PR Gil added this, because it is an outdated import:

ignore-packages = ['zope.app.component']

Actually, the try/except should be removed: the backward compatibility import of zope.app.component is not needed for Plone 5.2 or 6.0.

What I do when updating a package, if I remember to do it, is to grep for ImportError, and see if any old stuff is safe to be removed.

I did not yet think about it this way until now when I am writing this down, but what you are saying in that PR is: ignore zope.app.component because it should not be in the setup.py, but I will still keep the backward compatibility import. But if the BBB import is actually needed, it will fail because the dependency is not there. Then again, the initial import of zope.component won't fail, because it is in the dependencies, so zope.app.component can be ignored anyway. :-D

A different kind of problem would be this:

try:
   from something.optional import experiment
except ImportError:
    experiment = None
if experiment is not None:
    do stuff with it

In this case something.optional is not a hard dependency, and should be ignored. It could be nice if z3c.dependencychecker reports this.

But it may be fine to leave it up to the user to check for ImportErrors in the code, and see what their influence is.

[reinout]

I was worried a bit about an extra category we need to keep track of. So apart from regular imports and test imports we would also have "importerror-imports". Sounds like a big amount of work.

I agree that reporting about it is a very good idea. Perhaps just collecting and printing them at the start of the report? Is that enough?

[gforcada]

But it may be fine to leave it up to the user to check for ImportErrors in the code, and see what their influence is.

That's what I was kind of starting to think: the user should be informed that there are imports wrapped in a try/except ImportError and they should review what to do with it, but z3c.dependencychecker maybe should ignore those, as the user has been already informed 🤔

Thinking on it a bit more, we have this other dialect as well:

try:
    pkg_resources.get_distribution("Products.CMFCore")
except pkg_resources.DistributionNotFound:
    # If not present, returning None suffices
    def getToolByName(*args, **kw):
        return None

else:
    from Products.CMFCore.utils import getToolByName

Which might be worth checking, like ImportError ones.

[gforcada]

A slightly variation of this last example that uses pkg_resources is:

if pkg_resources.get_distribution("Products.CMFCore"):
  HAVE_FOO = True
else:
  HAVE_FOO = False

Which is sort of the same approach but does not use try/except, so more code to find that out 😓