Repository: nvim-lua/plenary.nvim (lua)

Issue:
context_manager.with should handle errors in `callable`
In Python, the `with` statement will catch any exceptions occurring within its body and apply the "exit" functionality before re-raising them. I think it could be useful for `context_manager.with` to behave in a similar way.

Basically, we'd wrap `callable` in `pcall`. It could look something like this:

```lua
local succeeded, result = pcall(callable, context)

-- Finish the coroutine/call obj:exit...

assert(succeeded, result)
return result
```

I'm new to plenary, so it's also very possible there's a reason it wasn't implemented like this that I'm not aware of :) If we want this functionality, I'd be happy to open a PR.

Pull request description:
This more closely emulates the behavior of the Python `with` statement,
which according to comments this is modeled off of.

With this change the context will always be closed, even if an error
occurs. This means that, for example, you can open a file without having
to worry about an error leaking the file handle. Or if you created a
context for creating temporary files and deleting them when done, you
wouldn't need to worry about failing to delete the temp file.

This change also re-enables disabled `context_manager` tests.

Fixes #340

Patch:
diff --git a/lua/plenary/context_manager.lua b/lua/plenary/context_manager.lua
index 57e7b22e..7d617da2 100644
--- a/lua/plenary/context_manager.lua
+++ b/lua/plenary/context_manager.lua
@@ -13,7 +13,7 @@ function context_manager.with(obj, callable)
     local ok, context = coroutine.resume(obj)
     assert(ok, "Should have yielded in coroutine.")
 
-    local result = callable(context)
+    local succeeded, result = pcall(callable, context)
 
     local done, _ = coroutine.resume(obj)
     assert(done, "Should be done")
@@ -21,6 +21,7 @@ function context_manager.with(obj, callable)
     local no_other = not coroutine.resume(obj)
     assert(no_other, "Should not yield anymore, otherwise that would make things complicated")
 
+    assert(succeeded, result)
     return result
   else
     assert(obj.enter)
@@ -28,9 +29,10 @@ function context_manager.with(obj, callable)
 
     -- TODO: Callable can be string for vimL function or a lua callable
     local context = obj:enter()
-    local result = callable(context)
+    local succeeded, result = pcall(callable, context)
     obj:exit()
 
+    assert(succeeded, result)
     return result
   end
 end


Test patch:
diff --git a/tests/plenary/context_manager_spec.lua b/tests/plenary/context_manager_spec.lua
index f52a4dea..54de82b9 100644
--- a/tests/plenary/context_manager_spec.lua
+++ b/tests/plenary/context_manager_spec.lua
@@ -1,81 +1,203 @@
---[[
-local lu = require("luaunit")
-
-local context_manager = require("plenary.context_manager")
-local debug_utils = require("plenary.debug_utils")
-local Path = require("plenary.path")
+local context_manager = require "plenary.context_manager"
+local debug_utils = require "plenary.debug_utils"
+local Path = require "plenary.path"
 
 local with = context_manager.with
 local open = context_manager.open
 
-local README_STR_PATH = vim.fn.fnamemodify(debug_utils.sourced_filepath(), ":h:h:h:h") .. "/README.md"
+local README_STR_PATH = vim.fn.fnamemodify(debug_utils.sourced_filepath(), ":h:h:h") .. "/README.md"
 local README_FIRST_LINE = "# plenary.nvim"
 
-TestContextManager = {}
+describe("context_manager", function()
+  it("works with objects", function()
+    local obj_manager = {
+      enter = function(self)
+        self.result = 10
+        return self.result
+      end,
 
-function TestContextManager:testWorksWithObj()
-  local obj_manager = {
-    enter = function(self)
-      self.result = 10
-      return self.result
-    end,
+      exit = function() end,
+    }
 
-    exit = function()
-    end,
-  }
+    local result = with(obj_manager, function(obj)
+      return obj
+    end)
 
-  local result = with(obj_manager, function(obj)
-    return obj
+    assert.are.same(10, result)
+    assert.are.same(obj_manager.result, result)
   end)
 
+  it("works with coroutine", function()
+    local co = function()
+      coroutine.yield(10)
+    end
 
-  lu.assertEquals(10, result)
-  lu.assertEquals(obj_manager.result, result)
-end
+    local result = with(co, function(obj)
+      return obj
+    end)
+
+    assert.are.same(10, result)
+  end)
 
+  it("does not work with coroutine with extra yields", function()
+    local co = function()
+      coroutine.yield(10)
 
-function TestContextManager:testWorksWithCoroutine()
-  local co = function()
-    coroutine.yield(10)
-  end
+      -- Can't yield twice. That'd be bad and wouldn't make any sense.
+      coroutine.yield(10)
+    end
 
-  local result = with(co, function(obj)
-    return obj
+    assert.has.error_match(function()
+      with(co, function(obj)
+        return obj
+      end)
+    end, "Should not yield anymore, otherwise that would make things complicated")
   end)
 
-  lu.assertEquals(10, result)
-end
+  it("reads from files with open", function()
+    local result = with(open(README_STR_PATH), function(reader)
+      return reader:read()
+    end)
 
-function TestContextManager:testDoesNotWorkWithCoroutineWithExtraYields()
-  local co = function()
-    coroutine.yield(10)
+    assert.are.same(result, README_FIRST_LINE)
+  end)
 
-    -- Can't yield twice. That'd be bad and wouldn't make any sense.
-    coroutine.yield(10)
-  end
+  it("reads from Paths with open", function()
+    local p = Path:new(README_STR_PATH)
 
-  lu.assertError(function()
-    with(co, function(obj)
-      return obj
+    local result = with(open(p), function(reader)
+      return reader:read()
     end)
+
+    assert.are.same(result, README_FIRST_LINE)
   end)
-end
 
-function TestContextManager:testOpenWorks()
-  local result = with(open(README_STR_PATH), function(reader)
-    return reader:read()
+  it("calls exit on error with objects", function()
+    local entered = false
+    local exited = false
+    local obj_manager = {
+      enter = function(self)
+        entered = true
+      end,
+
+      exit = function(self)
+        exited = true
+      end,
+    }
+
+    assert.has.error_match(function()
+      with(obj_manager, function(obj)
+        assert(false, "failed in callback")
+      end)
+    end, "failed in callback")
+
+    assert.is["true"](entered)
+    assert.is["true"](exited)
   end)
 
-  lu.assertEquals(result, README_FIRST_LINE)
-end
+  it("calls exit on error with coroutines", function()
+    local entered = false
+    local exited = false
+    local co = function()
+      entered = true
+      coroutine.yield(nil)
+
+      exited = true
+    end
 
-function TestContextManager:testOpenWorksWithPath()
-  local p = Path:new(README_STR_PATH)
+    assert.has.error_match(function()
+      with(co, function(obj)
+        assert(false, "failed in callback")
+      end)
+    end, "failed in callback")
 
-  local result = with(open(p), function(reader)
-    return reader:read()
+    assert.is["true"](entered)
+    assert.is["true"](exited)
   end)
 
-  lu.assertEquals(result, README_FIRST_LINE)
-end
---]]
+  it("fails from enter error with objects", function()
+    local exited = false
+    local obj_manager = {
+      enter = function(self)
+        assert(false, "failed in enter")
+      end,
+
+      exit = function(self)
+        exited = true
+      end,
+    }
+
+    local ran_callback = false
+    assert.has.error_match(function()
+      with(obj_manager, function(obj)
+        ran_callback = true
+      end)
+    end, "failed in enter")
+
+    assert.is["false"](ran_callback)
+    assert.is["false"](exited)
+  end)
+
+  it("fails from enter error with coroutines", function()
+    local exited = false
+    local co = function()
+      assert(false, "failed in enter")
+      coroutine.yield(nil)
+
+      exited = true
+    end
+
+    local ran_callback = false
+    assert.has.error_match(function()
+      with(co, function(obj)
+        ran_callback = true
+      end)
+    end, "Should have yielded in coroutine.")
+
+    assert.is["false"](ran_callback)
+    assert.is["false"](exited)
+  end)
+
+  it("fails from exit error with objects", function()
+    local entered = false
+    local obj_manager = {
+      enter = function(self)
+        entered = true
+      end,
+
+      exit = function(self)
+        assert(false, "failed in exit")
+      end,
+    }
+
+    local ran_callback = false
+    assert.has.error_match(function()
+      with(obj_manager, function(obj)
+        ran_callback = true
+      end)
+    end, "failed in exit")
+
+    assert.is["true"](entered)
+    assert.is["true"](ran_callback)
+  end)
+
+  it("fails from exit error with coroutines", function()
+    local entered = false
+    local co = function()
+      entered = true
+      coroutine.yield(nil)
+
+      assert(false, "failed in exit")
+    end
+
+    local ran_callback = false
+    assert.has.error_match(function()
+      with(co, function(obj)
+        ran_callback = true
+      end)
+    end, "Should be done")
+
+    assert.is["true"](entered)
+    assert.is["true"](ran_callback)
+  end)
+end)