Implement @impl directive by samsondav · Pull Request #6031 · elixir-lang/elixir

samsondav · 2017-04-23T08:19:02Z

Implement @impl directive

- See: #5734

josevalim · 2017-04-23T08:53:34Z

                                       unquote(stack), unquote(line))

-      :lists.member(name, [:moduledoc, :typedoc, :doc]) ->
+      :lists.member(name, [:moduledoc, :typedoc, :doc, :impl]) ->


It is better to not rely on the line number. That's not a general feature of Elixir module attributes and as such it should be relied on very rarely.

josevalim · 2017-04-23T08:54:20Z

According to the build it is working: https://travis-ci.org/elixir-lang/elixir/jobs/224857673#L237

It is just failing elsewhere inside the check_impl function.

samsondav · 2017-04-23T13:42:37Z

@josevalim This is about as far as I can go on my own before getting feedback, would you mind taking a look?

samsondav · 2017-04-23T14:23:50Z

Perhaps we should actually raise instead of warn here?

samsondav · 2017-04-23T14:25:46Z

@josevalim The original proposal specified an @impl true variant but I actually prefer the explicitness of @impl Module and think we should enforce this form.

Perhaps we could ship the @impl Module specific case and see how people like it. If it seems obvious that @impl true would be nice to have in addition, we can easily add it later. Conversely, adding it now and removing it later would be much more difficult.

@samphilipd I am already decided on making @impl true part of the implementation so we should go ahead with it.

josevalim · 2017-04-23T16:33:32Z

Please use pattern matching to extract line, file, module and others out.

%{module: module, line: line, file: file} = env

josevalim · 2017-04-23T16:34:36Z

Why would we need to change store_typespec? :)

You are correct. It's not necessary.

josevalim · 2017-04-23T16:35:46Z

For internal attributes, such as :impl_required, you can namespace it as{:elixir, :impl_required} to avoid conflicts with user attributes.

josevalim · 2017-04-23T16:36:19Z

Nothing. :) This is during bootstrap, so we don't care. If there are warnings, the modules will be recompiled after bootstrap, so we get them anyway.

josevalim · 2017-04-23T16:37:00Z

Can you please send those changes in another PR? :)

samsondav · 2017-04-24T07:44:02Z

@josevalim I addressed your comments, this is ready for a second round of feedback.

samsondav · 2017-04-24T07:47:45Z

There's probably room for refactoring here and combining logic between msg_impl_true_but_function_is_not_callback and msg_impl_behaviour_but_function_is_not_callback

samsondav · 2017-04-24T07:48:50Z

This test doesn't work. Is there another way to get the @doc of a module?

samsondav · 2017-04-24T07:59:45Z

This should always be a warning rather than an outright ArgumentError?

samsondav · 2017-04-24T08:03:15Z

It might make sense to merge this somehow with the true case since they both duplicate this callbacks_with_behaviour logic.

samsondav · 2017-04-24T08:05:35Z

This to_sentence function seems a bit too much for Module, but I couldn't think of another way to nicely present the warnings.

samsondav · 2017-04-25T13:05:16Z

@josevalim OK I'm fairly happy with this, only two questions:

Should we ever raise in any case? Or always warn?
See the test at the bottom of warnings_test.exs, I can't see how to implement this case.

samsondav · 2017-04-28T09:26:40Z

@josevalim just a polite nudge for a review on this :)

- See: elixir-lang#5734

- Closes elixir-lang#5734

josevalim · 2017-05-09T10:47:49Z

+
+  You may pass either `false`, `true`, or a specific behaviour to `@impl`.
+
+    defmodule Foo do


Please indent with four spaces.

josevalim · 2017-05-09T10:51:55Z

Thank you @samphilipd! I will merge it locally and tidy up what is left. :)

josevalim · 2017-05-09T14:28:23Z

This has been merged and push to master. In order for the last test to pass, we had to change when the check happens, and this one done in a another commit: 92e5d04

Thank you so much! ❤️ 💚 💙 💛 💜

samsondav mentioned this pull request Apr 23, 2017

Introduce @impl attribute and improve defoverridable #5734

Closed

josevalim reviewed Apr 23, 2017

View reviewed changes

samsondav force-pushed the implement_impl branch from d28bb2b to 3eb8621 Compare April 23, 2017 14:01

samsondav commented Apr 23, 2017

View reviewed changes

Comment thread lib/elixir/lib/module.ex Outdated

Copy link
Copy Markdown

Author

samsondav Apr 23, 2017

Choose a reason for hiding this comment

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

Perhaps we should actually raise instead of warn here?

samsondav commented Apr 23, 2017

View reviewed changes

samsondav force-pushed the implement_impl branch from d5e7e3e to 1764f8d Compare April 23, 2017 15:27

josevalim reviewed Apr 23, 2017

View reviewed changes

Comment thread README.md Outdated

Copy link
Copy Markdown

Member

josevalim Apr 23, 2017

Choose a reason for hiding this comment

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

Can you please send those changes in another PR? :)

samsondav commented Apr 24, 2017

View reviewed changes

Comment thread lib/elixir/lib/module.ex Outdated

Copy link
Copy Markdown

Author

samsondav Apr 24, 2017

Choose a reason for hiding this comment

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

This should always be a warning rather than an outright ArgumentError?

samsondav commented Apr 24, 2017

View reviewed changes

samsondav force-pushed the implement_impl branch 4 times, most recently from 0924fcf to db5186c Compare April 25, 2017 13:01

samsondav force-pushed the implement_impl branch from 82b8235 to 078fdbd Compare April 25, 2017 14:36

samsondav changed the title ~~Implement impl~~ Implement @impl directive Apr 25, 2017

samsondav force-pushed the implement_impl branch 2 times, most recently from 03fc3ce to 960d503 Compare April 28, 2017 09:24

samsondav force-pushed the implement_impl branch from 960d503 to 5d01187 Compare April 28, 2017 09:28

Sam Davies added 3 commits May 6, 2017 14:43

Implement @impl directive

9d93427

- See: elixir-lang#5734

Rename to value for better readability

79c03f3

- Closes elixir-lang#5734

Improve impl tests

2252530

samsondav force-pushed the implement_impl branch from 5d01187 to 2252530 Compare May 6, 2017 13:44

josevalim reviewed May 9, 2017

View reviewed changes

josevalim closed this May 9, 2017


		You may pass either `false`, `true`, or a specific behaviour to `@impl`.

		defmodule Foo do

Conversation

samsondav commented Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Apr 23, 2017

Uh oh!

samsondav commented Apr 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsondav Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsondav commented Apr 24, 2017

Uh oh!

samsondav Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsondav Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsondav commented Apr 25, 2017

Uh oh!

samsondav commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented May 9, 2017

Uh oh!

josevalim commented May 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

samsondav commented Apr 23, 2017 •

edited

Loading

samsondav Apr 23, 2017 •

edited

Loading

samsondav Apr 24, 2017 •

edited

Loading

samsondav Apr 24, 2017 •

edited

Loading