Handle gracefully raise X from a missing dependency

[ZanSara]

Fixes #4

---

The PR changes the behavior of the following snippet (using langdetect as my missing dependency):

from generalimport import generalimport
generalimport("langdetect")

from langdetect import LangDetectException as MyException
raise MyException

from

TypeError: exceptions must derive from BaseException

to:

Traceback (most recent call last):
  File "/path-to-project/snippet.py", line 44, in <module>
    raise MyException
generalimport.exception.missing_exception.<locals>.MissingOptionalDependencyException: Optional dependency langdetect (required by 'LangDetectException') was used but it isn't installed.

It also handles a different scenario (the one that led me to this bug to begin with) which looks like this:

from generalimport import generalimport
generalimport("langdetect")

from langdetect import LangDetectException, detect

try:
    language = detect(document)
except LangDetectException:
    logger.warning("Langdetect cannot detect the language of document: %s", document)

If langdetect was not installed, the except LangDetectException: call would raise:

TypeError: catching classes that do not inherit from BaseException is not allowed

This scenario now also raises:

generalimport.exception.missing_exception.<locals>.MissingOptionalDependencyException: Optional dependency langdetect (required by 'LangDetectException') was used but it isn't installed.

Probably not the best error name, but at least can be catched and improved upon.

I added no tests for now but I believe there should be. I'll wait for you to review the changes and maybe to give me a few hints how you want the tests to look like.

[Mandera]

Relying on the name is an interesting idea! If it wasn't for PEP8 I wouldn't have been too excited about it, would have preferred relying on some magical dunder method that's only called by raise, alas, such a method probably doesn't exist, so this could work!

Using item in the error message is a nice touch

However, I'm not too excited about the other implementation

• The new missing_exception function doesn't seem necessary, could we go through error_func instead?
  • I'd really like to stay DRY, I'll try to list these principles in Create a CONTRIBUTING.md definition generalpackager#81
• The new self.trigger also doesn't seem necessary, and I'm not sure it should even default to spec.name
  • It doesn't really make sense though to store the item value in FakeModule since we cannot be sure the latest yielded object is the one that will be called, see this example:

from langdetect import hello
from langdetect import hi

hello()

generalimport.exception.MissingOptionalDependency: Optional dependency 'langdetect' (required by 'hi') was used but it isn't installed.

It says "required by 'hi'". I think this has potential though! Could probably create a protected _error_func that error_func relays to and then have _error_func be directly callable with the optional item parameter

[ZanSara]

Relying on the name is an interesting idea! If it wasn't for PEP8 I wouldn't have been too excited about it, would have preferred relying on some magical dunder method that's only called by raise, alas, such a method probably doesn't exist, so this could work!

I also wasn't very happy with this, there's always going to be some library that misbehaves (even though that would be their fault 😁 ). I put those patterns in a constant so in case of need people could always modify it at runtime. Ugly, but it's an emergency solution after all. If I did the same matching inline in L28 it would have been impossible to extend.

I really looked for those dunder methods too... but no way. Exception matching is baked into CPython: i had to dig into the C source to make sure and yep, it's done in C. No way around it.

The new missing_exception function doesn't seem necessary,

I found it necessary to generate the exception class on the fly because there's no other (evident) way to pass the dependency value to it. __getattr__ must return a class for the whole thing to work, not an instance, so this was the cleanest way to make sure the name of the dependency was carried over. I might be wrong though. Feel free to simply return the class, like:

def __getattr__(self, item):
    if any(str(item).endswith(pattern) for pattern in EXCEPTION_NAMING_PATTERNS):
        return MissingOptionalDependency  # Can't return MissingOptionalDependency("the error message"): it must be a class.
         ...

to see how the error looks like. Of course if you find a better way I'm happy with anything that works!

The new self.trigger also doesn't seem necessary

Fair, that was mostly for me to debug, I admit. I'll spend a bit more time to see if there's a neater way to make it work, otherwise it's going to disappear from this PR.

[ZanSara]

I might have found a nice way of keeping trigger while fixing the bug you highlight above. I push that in the next commit.

It makes this snippet:

from generalimport import generalimport
generalimport("langdetect")

from langdetect import hello
from langdetect import hi

hello()

return this message:

generalimport.exception.MissingOptionalDependency: Optional dependency 'langdetect' (required by 'hello') was used but it isn't installed.

[ZanSara]

In the very opinionated changes commit I also added some "cosmetic" changes that I actually used for most of the development of this feature, and that I thought you might like. If you don't like them, no issue! I will absolutely revert them back. They're quite opinionated and of little use except ease my debugging, so I won't miss them too much if you push them back 😄

[Mandera]

Interesting ideas! Could I please ask you to separate these changes into one or two other PRs before I process them? My IDE is looking like a Christmas tree haha

[ZanSara]

Alright the diff should be back to a manageable size 😄

[Mandera]

Adding some changes to this PR and merging master onto it

[Mandera]

I think code is OK but struggling with dev workflow. I thought that the "missing-exceptions" branch would be available on my repo but I'm guessing it has to use the current fork too

[ZanSara]

Hey @Mandera! Yes that's normal, missing-exceptions should not be available on your main repository. However, you should have full access to my fork.

If you want/need to see the branch on your repo you can either:

• Merge my fork's branch onto an equally named branch of your repo before merging to master (may be automated, but be careful with this), or
• Give me write access to your repo (even more dangerous than the previous one ofc).

As a third, very rough alternative you can use GitHub's own web editor to edit my PR's code without checking out my repo, but that might not be what you need.

---

On top of this, I have one last bit of unsolicited advice 😅 By looking at your CI it seems to me like you would like to test all your libraries for integration every time anything happens on any of them. While that is surely useful, why replicating the effort on every single project's CI? That's a lot of duplication to handle.

I'd recommend setting up an additional repo with only the integration tests, and run those tests with a repository_dispatch hook: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#repository_dispatch. In this way, every time an event of your choice occurs on any of your repos, you can trigger the tests there and make sure they pass, and you don't need to keep or clone these tests anywhere else.

In addition, as a contributor it would be really weird if I saw tests failing for unrelated projects on my PRs. I think this is information that you can use to decide whether to merge a PR or not, but you should not directly force everyone to fix failing tests on other repos in order to get their code merged in this one. Otherwise these libraries are just pretending to be self-sufficient and are instead a monolith that I have to understand from top to bottom in order to modify it.

Sorry if I've been a bit direct 😅 but I believe this is quite important for the success of this project.

[Mandera]

Ah I understand! No that's totally fine! I really appreciate your advice 👍

I think it's a good idea that the workflow only runs unit tests for its own repo. I definitely want each repo to be self-sufficient! It's an easy change for the dev workflow.

To change it in the main workflow (push to master) I'd have to rethink how the repos are synced and published, a bit of a bigger job but definitely on the radar!

I like the idea of a separate repo with those (integration?) tests, perhaps the syncing, version bumping, and publishing could take place there 🤔

[Mandera]

Ah I understand! No that's totally fine! I really appreciate your advice 👍

I think it's a good idea that the workflow only runs unit tests for its own repo. I definitely want each repo to be self-sufficient! It's an easy change for the dev workflow.

To change it in the main workflow (push to master) I'd have to rethink how the repos are synced and published, a bit of a bigger job but definitely on the radar!

I like the idea of a separate repo with those (integration?) tests, perhaps the syncing, version bumping, and publishing could take place there 🤔

[Mandera]

Tests passed except for the two from

• Failing tests in test suite #20

Not sure why that's happening. I'll go ahead and merge this because I believe those tests are passing on the main workflow, let's see!