Support :error tuples as a valid return value in event handlers

[marcelotto]

Maybe I'm missing something, but is there a way to stop parsing with a :error tuple which is directly passed through as the overall result?

I'm using Saxy for writing an RDF/XML parser which imposes some further restrictions on a valid document. So, I'd like to return a :error tuple in the event handlers when encountering an error, which I'd expect to be passed through directly as the result of Saxy.parse_string/3, but currently this results in this:

{:error,
    %Saxy.ParseError{
      __exception__: true,
      binary: nil,
      position: nil,
      reason: {:bad_return, {:start_element, {:error, "my custom error"}}}
    }
  }
}

I'm aware of the :halt and :stop return patterns, which could be used for this scenario of course, but neither of them feels quite right:

• with :stop the :error tuple is wrapped in a :ok tuple
• with :halt the :error tuple is wrapped in a three element :halt tuple

What do you think of supporting :error tuples as an additional return pattern in the handlers, which will become the result of Saxy.parse_string/3 directly?

[qcam]

I think the idea behind the :stop | :halt | :ok is to give the users control of the parsing flow of the XML document being parsed. It was not really about the semantics of the documents though.

IMHO, :stop was built exactly for this case. We are parsing an XML document and sure that it does not makes sense to continue any more. Therefore, we tell the parser to stop with a value {:error, :invalid}. Saxy returns {:ok, {:error, :invalid}}, with :ok marker saying that nothing wrong on Saxy (XML parsing was OK), then we use {:error, :invalid} to know the RDF/XML document was semantically invalid.

Regarding adding :error tuple, I'm not entirely sure. It might perhaps complicate the expectation of Saxy.parse_string/3 for the error case 🤔. What values would you think adding :error would bring to the table?

P/S: I do have some small regrets, it would probably be more intuitive if the instructions were :cont | :stop | :halt.

[marcelotto]

Regarding adding :error tuple, I'm not entirely sure. It might perhaps complicate the expectation of Saxy.parse_string/3 for the error case 🤔. What values would you think adding :error would bring to the table?

I would argue exactly the opposite: it would make handling of the error case simpler and more uniform. If a user doesn't care for the exact error type he can simply have one error clause.

case Saxy.parse_string(...) do
  {:ok, result} -> ...
  {:error, error} -> raise error
end

But if he likes to differentiate the error type he can still do it this way:

case Saxy.parse_string(...) do
  {:ok, result} -> ...
  {:error, %Saxy.ParseError{} = e} -> ... 
  {:error, %RDF.XML.ParseError{} = e} -> ...
  {:error, %Other.Error{} = e} -> ...
end

[qcam]

Could I propose starting with :stop to you for now, since I'd prefer to keep the handler API simple. But let's leave this discussion open to see if more people are interested in this change. What do you think?

[marcelotto]

Sure, no problem. I'm already working with the current approach. It was just a suggestion for improvement.

But I actually went with :halt, since returning an error with :stop hampers a simple use of with:

with {:ok, result} <- Saxy.parse_string(...) do
  # we're getting here with result being an error
end

[thiagomajesk]

I want to leave +1 here to support this change. Found very confusing that {:ok, reason} is returned even when receiving a {:stop, reason} signal by handlers. Looking from the public API perspective it's very counterintuitive.

Besides that, I think that since Saxy processing is an abstraction, the end-user shouldn't care whether the error has ocurred on the engine level, the handling level, or the XML level. I do think @marcelotto is right on this, the best way to encapsulate the possible error messages is to just return a plain {:error, reason} with a meaningful data structure that presents further the details (if we care about them at all).