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

Raise File.Error exception on non existing file #143

Merged

Conversation

gpopides
Copy link
Contributor

@gpopides gpopides commented Mar 5, 2022

refs #137

@mruoss
Copy link
Collaborator

mruoss commented Mar 6, 2022

Thanks for this. It does solve the issue at hand. However, File.read() also returns other tuples. So maybe there's other cases to deal with at some point. But this solves already the most common one.

Hmm... you should not have such a hard time with the pipeline. Let me fix that on develop branch so you can rebase.

@gpopides
Copy link
Contributor Author

gpopides commented Mar 6, 2022

Hey, i thought about the other tuples but i was not sure if File.Error is the right exception for all of them. Let me know if you want to address this in this PR

@mruoss
Copy link
Collaborator

mruoss commented Mar 6, 2022

You can rebase now and pipeline should be fine.
As for your question, I'm undecided on how to deal with this. One possibility is to refactor so that from_file/2 calls from_file!/2, rescues exceptions and turns them into tuples. But afaik that's generally not a very good idea because exceptions should be avoided... Maybe a topic for the elixir forum...

@gpopides
Copy link
Contributor Author

gpopides commented Mar 6, 2022

File.read!/1 returns always File.Error, so i think always raising File.Error from from_file!/2 would be an option.

@mruoss
Copy link
Collaborator

mruoss commented Mar 6, 2022

What if in render where we know the error comes from File.read we turn it into a {:error, %File.Error{...}} tuple with :reason set accordingly?

Also see File.read!

@gpopides
Copy link
Contributor Author

gpopides commented Mar 6, 2022

from_file also may return a yaml parsing error. Do we want to distinguish handling between File.Error and Yaml.ParsingError ?

@mruoss
Copy link
Collaborator

mruoss commented Mar 6, 2022

If we turn the {:error, atom()} tuple in render/2 into a {:error, %File.Error{}} tuple, we can leave the code in from_file! as is. It's just gonna raise the File.Error (or Yaml.ParsingError if is returned):

@@ -17,6 +17,18 @@ defmodule K8s.Client.DynamicHTTPProvider do
defdelegate headers(method, request_options), to: K8s.Client.HTTPProvider

@impl true
@spec handle_response(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated and should not be necessary now... right?

@mruoss mruoss merged commit f5ecef8 into coryodaniel:develop Mar 7, 2022
@mruoss
Copy link
Collaborator

mruoss commented Mar 7, 2022

Thanks @gpopides. Committed a slight change in 6990b83.

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.

None yet

2 participants