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

Remove unnecessary condition from pattern matching #39

Closed
wants to merge 1 commit into from

Conversation

asiniy
Copy link
Contributor

@asiniy asiniy commented Aug 9, 2016

I think it's not needed here. WDYT?

I think it's not needed here. WDYT?
@keichan34
Copy link
Owner

Hmm.. well, is_atom(nil) is true, so I wanted to be extra sure. I see that if endpoint is nil in that function, it will call Phoenix.Router.Helpers.url(nil, nil), which results in an error:

iex(1)> Phoenix.Router.Helpers.url(nil, nil)
** (UndefinedFunctionError) function nil.url/0 is undefined or private
    nil.url()

I think we can leave it in, and if there's a case where endpoint is nil, show a more descriptive error message? With the current implementation there is just the match error. What do you think?

@asiniy
Copy link
Contributor Author

asiniy commented Aug 9, 2016

@keichan34 kinda unpredictable behaviour of is_atom(true) here. You're absolutely right here, I'm closing the PR.

By the way, I asked a question at SO: http://stackoverflow.com/questions/38852905/why-is-atomnil-equals-to-true-in-elixir

@asiniy asiniy closed this Aug 9, 2016
@asiniy asiniy deleted the patch-1 branch August 9, 2016 14:01
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