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

Does OK.with handle raised errors? #40

Closed
seivan opened this issue Dec 13, 2017 · 13 comments
Closed

Does OK.with handle raised errors? #40

seivan opened this issue Dec 13, 2017 · 13 comments

Comments

@seivan
Copy link

seivan commented Dec 13, 2017

https://github.com/CrowdHailer/OK#oktry

OK.try do
  user <- fetch_user(1)             # `<-` operator means func returns {:ok, user}
  cart <- fetch_cart(1)             # `<-` again, {:ok, cart}
  order = checkout(cart, user)      # `=` allows pattern matching on non-tagged funcs
  saved_order <- save_order(order)
after
  response(:created)                # Value will be returned unwrapped
rescue
  :user_not_found ->
    response(:not_found)
  :could_not_save ->
    response(:internal_server_error)
end

Does that mean the cases under rescue are raised errors, or errors in {:error, reason} ?

@CrowdHailer
Copy link
Owner

rescue is for {:error, reason}.
Unfortunately there is a very limited selection of keywords I can use for starting new code blocks.

@seivan
Copy link
Author

seivan commented Dec 13, 2017

No, I get the problem, but don't you think "rescue" is confusing in this case?
I would have guessed it would have caught raised issues.

Not related, but are the values defined under the try block available inside rescue?
A pattern I tend to use from using Either in swift is log the error with the previous values defined.

E.g if parsing a JSON string loaded from a file fails.
I'd like to know the file name as well as the JSON content itself otherwise I would have to wrap every method that might cause an error myself and that's tedious.

static fileprivate func parseJSON(fromFileContent fileContent:FileSystemLoader.FileContent<String>) throws -> Parser.UnfilteredContent {
        return try Either<Error, FileSystemLoader.FileContent<String>>
            .right(fileContent)
            .map { $0.content }
            .map { $0.data(using: String.Encoding.utf8) }
            .map { try $0.throwingIfNil(Parser.ParseError.failedEncodingFileContentToData(fileContent)) }
            .map { try JSONSerialization.jsonObject(with: $0, options: []) }
            .mapLeft { throw Parser.ParseError.failedParsingFromFileContent(fileContent, $0) }
            .map { FileSystemLoader.FileContent(with: (raw:fileContent.content, processed:$0 as AnyObject), from: fileContent.file) }
            .unwrapError()

    }
    ```

@seivan
Copy link
Author

seivan commented Dec 13, 2017

Turns out it's not

  def ok do
    OK.try do
      file    <- "priv/sample.json" |> File.read
      content <-  file |> Poison.Parser.parse
    after
      IO.inspect content
    rescue
      error -> IO.inspect file
    end
    
  end

It would be nice to have access to have a list of the content before the failure without inserting them to a list outside the block

@CrowdHailer
Copy link
Owner

That sounds like an interesting challenge. If the rescue block is called because File.read failed then what would the value of file be.

yes I agree it's confusing. I think the only solution I have is a nice explanation on why exceptional cases (bad input) are not the same as programmer errors. I use OK to handle the former and raised errors just end up in by bug tracker.
Not just an elixir problem but using defexception in modules called FooError is also rather unhelpful

@seivan
Copy link
Author

seivan commented Dec 13, 2017

@CrowdHailer I think we're venturing outside the scope of this library, my bad.

What I am looking for is not something where I match on a specific error, but a specific call.
I think code makes for easier explanation so, lets draw some pseudo code.

  def ok do
    OK.try do
      content    <- filename |> File.read  as: {LoadingFile, filename}
      parsed_content <-  file |> Poison.Parser.parse as: {ParsingContent: [content, filename]}
    after
      IO.inspect parsed_content
    rescue
     {LoadingFile: filename} -> IO.inspect filename
     {ParsingContent: [content, filename]} -> IO.inspect content
    end
  end

Obviously it would be nice to also match on the error, but usually for me, I debug better when I can know where something failed and what it used to fail on.
Error messages like {:error, {:invalid, "a", 0}} doesn't tell me much but I understand better when I get to learn that the content that was parsed just contained asdada in the filename sample.json

@seivan
Copy link
Author

seivan commented Dec 13, 2017

To be honest, I actually also prefer the piping way (like the Swift sample above) than using the "with" syntax.

Some pseudo code

operation = content
~~> happy_path
~~> until_something
~~> goes_wrong
~~> and_let_me_have_all
~~> the_info_to_match_on_the_wrong


case operation.result. do
    {:ok, value} -> IO.inspect value 
    {:error, reason} -> IO.inspect reason
end

case operation.error. do
    {:happy_path, [content | _]}
        -> IO.inspect "Error occured on happy path with content  "#{v}" 
    {: until_something, [result_from_happy_path | x]}  
        -> IO.inspect "Error occured on : until_something content  "#{result_from_happy_path}" 

    {: goes_wrong, [result_from_until_something | x]}  
        -> IO.inspect "Error occured on : goes_wrong content  "#{result_from_until_something}" 

end

@CrowdHailer
Copy link
Owner

That is much closer to the swift example you shared before.
just to double check you know that OK has an ~>> operator?

result = content
~>> happy_path
~>> until_something
~>> goes_wrong
~>> and_let_me_have_all
~>> the_info_to_match_on_the_wrong

case result do
  {:ok, v} ->
  {:error, v} -> 
end

@seivan
Copy link
Author

seivan commented Dec 13, 2017

I am, in fact, I was looking at it as a base for something new. Maybe one could wrap lhs ( and unwrap before the next ~>> pipe.
That way you always end up with wrapped struct or tuple that has additional information about its error.

@seivan
Copy link
Author

seivan commented Dec 13, 2017

https://github.com/CrowdHailer/OK/blob/master/lib/ok.ex#L141

I am guessing the first two lines here have something to do with the end clause

 value = quote do: value
    args = [value | args]
#.......
 unquote({call, line, args})

On inspecting value, and args, I find it to be empty but it should contain my argument ([1]) could you explain how that actually works?

@seivan
Copy link
Author

seivan commented Dec 13, 2017

What's your take on something like

content = "filename.json"
operation = 
content
|> Ok.pipable
~>> happy_path, as: :happy
~>> until_something
~>> goes_wrong, as: :wrongy

case operation.result. do
    {:ok, value} -> IO.inspect value 
    {:error, reason} -> IO.inspect reason
end

case operation.error. do
    {:happy, [filename | _]}
        -> IO.inspect "Error occured on happy path with filename  "#{v}" 
    {: until_something, [result_from_happy_path | x]}  
        -> IO.inspect "Error occured on : until_something content  "#{result_from_happy_path}" 
    {: wrongy, [result_from_until_something | x]}  
        -> IO.inspect "Error occured on : goes_wrong content  "#{result_from_until_something}" 

end

@CrowdHailer
Copy link
Owner

I'm not convinced I see that value for my code but I have got familiar with how OK is now. I would be interested in the experiment, certainly. perhaps get a few more opinions on the discussion would be good too

@seivan
Copy link
Author

seivan commented Dec 14, 2017

That sounds good. You can feel free to close this if you're thinking of a different avenue to continue the discussion on.
I also want to point out that I agree with you:

Tempted to say all elixir funcs should return a result tuple. {:ok, 4} = 2 + 2

I actually think Elixir would have been better if it treated everything as a "monad", even if it's just a tuple.

@CrowdHailer
Copy link
Owner

@seivan thanks for your points here. Closing as I don't think this issue has any next action. apart from perhaps more docs/a blog post explaining the points but (speaking for my self) that's not happening soon and I already have a similar issue for that

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

No branches or pull requests

2 participants