Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle undefined modules when patching and raise UndefinedFunctionError #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xpgdk
Copy link

@xpgdk xpgdk commented Jun 27, 2024

I have noticed that trying to patch non-existing modules, results in an error, that may be quite difficult to understand.
The following code

patch(NoSuchModule, :function_that_does_not_exist, fn _x -> 42 end)

will result in the following error

     ** (MatchError) no match of right hand side value: {:error, :nofile}
     code: patch(NoSuchModule, :function_that_does_not_exist, :patched)
     stacktrace:
       (patch 0.13.1) lib/patch.ex:492: Patch.patch/3
       (patch 0.13.1) lib/patch.ex:514: Patch.patch/3
       test/user/patch/unknown_module_test.exs:8: (test)

The first couple of times I saw this, I had absolutely no clue about what was going on. Only by looking into the patch source code, it became apparent, that I was doing something wrong.

This PR adds a simple check that handles the {:error, :nofile} case, and emits an error message that should be easier to understand:

     ** (UndefinedFunctionError) function NoSuchModule.function_that_does_not_exist/0 is undefined (module NoSuchModule is not available)
     code: patch(NoSuchModule, :function_that_does_not_exist, :patched)
     stacktrace:
       (patch 0.13.1) lib/patch.ex:497: Patch.patch/3
       (patch 0.13.1) lib/patch.ex:514: Patch.patch/3
       test/user/patch/unknown_module_test.exs:8: (test)

@ihumanable
Copy link
Owner

I'm ok with raising an exception here, I'm not sure about UndefinedFunctionError though.

Here's the reasons I'm less inclined to use UndefinedFunctionError:

  1. It's not very descriptive to the situation. The changeset here handles the case that Patch.Mock.module/1 returns an error, the thing that's undefined is actually the module not the function.
  2. Well, you may retort, if the module is missing then so is the function. I agree with this but the implementation will happily no-op on a missing function.

This leaves us in a situation where the only time patch raises an UndefinedFunctionError is when the module is undefined and not when the function is undefined which also seems like weird behavior.

I think it would make sense to do either of the following:

  1. Introduce a custom error here which more clearly indicates the failure. UndefinedModuleError. Use that for this case and retain the current behavior of no-op on a missing function.
  2. Change the behavior on missing function to raise and then this can remain UndefinedFunctionError.

Are you interested in either of those alternatives?

@xpgdk
Copy link
Author

xpgdk commented Sep 5, 2024

I completely agree that UndefinedFunctionError is not a good exception to throw. I only chose it, because it aligns with what Elixir itself does:

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:20:20] [ds:20:20:10] [async-threads:1] [jit:ns]

Interactive Elixir (1.17.0) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> NonExistingModule.call()
** (UndefinedFunctionError) function NonExistingModule.call/0 is undefined (module NonExistingModule is not available)
    NonExistingModule.call()
    iex:1: (file)
iex(1)> 

I think adding a custom UndefinedModuleError is the most clear thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants