Skip to content

Conversation

eksperimental
Copy link
Contributor

It will raise with ArgumentError if :version key is not provided in config.

/cc @ericmj

@eksperimental eksperimental changed the title Raise if version is not provided in Docs task Raise if version is not provided in Docs task config May 23, 2017
end

defp fetch_version!(config) do
try do

Choose a reason for hiding this comment

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

Prefer using an implicit try rather than explicit try.

end

defp fetch_version!(config) do
Keyword.fetch!(config, :version)
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to raise, then we should use Keyword.fetch + pattern matching.

Copy link
Member

Choose a reason for hiding this comment

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

I.e. we shouldn't rescue exceptions. I believe when @ericmj mentioned Keyword.fetch! he was not expecting a custom error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this was really wrong. Too tired I guess 😫

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@ericmj
Copy link
Member

ericmj commented May 23, 2017

I don't think we need a custom error message since it should always be caught by the compiler. Just using Keyword.fetch!/2 should be fine.

@eksperimental
Copy link
Contributor Author

@ericmj It will be caught. I thought it was better to give a more meaningful message. But it's up to you guys

@josevalim josevalim merged commit 931c7ee into elixir-lang:master May 23, 2017
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@eksperimental eksperimental deleted the force_version branch May 23, 2017 13:19
josevalim pushed a commit that referenced this pull request May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants