Skip to content

Validate the value of the @on_load attribute#6038

Merged
whatyouhide merged 2 commits into
elixir-lang:masterfrom
whatyouhide:validate-on-load
Apr 27, 2017
Merged

Validate the value of the @on_load attribute#6038
whatyouhide merged 2 commits into
elixir-lang:masterfrom
whatyouhide:validate-on-load

Conversation

@whatyouhide
Copy link
Copy Markdown
Member

I think this could be a way to implement this check:

{bad_on_load,Term}: badly formed on_load attribute

from #5800. Thoughts?

Comment thread lib/elixir/lib/module.ex

defp preprocess_attribute(:on_load, atom) when is_atom(atom) do
{atom, 0}
defp preprocess_attribute(:on_load, value) do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The @on_load attribute can be either {:fun, 0} or :fun. So I think we should probably leave this clause intact and add a clause that check when it is not in the tuple format above.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh missed that, that's right. We don't document it in the docs for Module though, should we?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should, yes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Got it, updated. I didn't follow your suggestion but went with a single clause where we validate the value, similar to what we do for other attributes. Let me know if you prefer your suggestion and I'll change it :)

@whatyouhide whatyouhide merged commit 0841f5e into elixir-lang:master Apr 27, 2017
@whatyouhide whatyouhide deleted the validate-on-load branch April 27, 2017 09:55
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.

2 participants