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

Ambiguous variables should probably check arity #74

Closed
philipgiuliani opened this issue May 16, 2016 · 4 comments
Closed

Ambiguous variables should probably check arity #74

philipgiuliani opened this issue May 16, 2016 · 4 comments
Assignees

Comments

@philipgiuliani
Copy link

philipgiuliani commented May 16, 2016

Josè has changed the generators in phoenix to use struct as parameter instead of model:

def changeset(struct, params \\ %{}) do

Credo doesn't like that:

[W] ↗ Parameter `struct` has same name as the `Kernel.struct` function.

So i asked Jose about it and he told me that Kernel only implements struct/2, so it is ok to use struct/0 here.

I think it would be right that NameRedeclarationByAssignment is actually checking the exact method. So not just struct but struct/2

@philipgiuliani philipgiuliani changed the title Ambiguous variables Ambiguous variables should probably check arguments May 16, 2016
@rrrene
Copy link
Owner

rrrene commented May 17, 2016

That's a valid argument.

I am a bit uncertain whether or not to keep these checks at all. Originally you could write something like

defp server(https \\ false) do
  # ... code ...
end

# somewhere in a function:
server = server(true)

Now, with a regular call to server it might have become unclear if you were calling the variable or the function (using default arguments). But since Elixir 1.2 you would have to call server() to call the function, so this check might be outdated ...

Long story short, you're suggestion to check the arity is a very good one to solve those problems. 👍

@rrrene rrrene self-assigned this May 17, 2016
@rrrene rrrene added this to the Release v0.4.0 milestone May 17, 2016
@philipgiuliani philipgiuliani changed the title Ambiguous variables should probably check arguments Ambiguous variables should probably check arity May 24, 2016
@rrrene rrrene removed this from the Release v0.4.0 milestone Jun 14, 2016
@rrrene rrrene closed this as completed in 8728ef9 Jun 20, 2016
@philipgiuliani
Copy link
Author

Woho, awesome! :)

@rrrene
Copy link
Owner

rrrene commented Jun 20, 2016

v0.4.5 is live and contains this fix.

@net
Copy link

net commented Jun 20, 2016

Yay!

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

3 participants