Adds support for optionalcallback and optionalmacrocallback #4037

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@stevedomin
Contributor

The optionalcallback or optionalmacrocallback attribute also defines the
Erlang optional_callbacks attribute.

If a (macro)callback has been defined as optional(macro)callback and
(macro)callback we emit a warning and defines the optional_callbacks
attribute.

http://www.erlang.org/doc/design_principles/spec_proc.html#id74952

Fixes #3980

@stevedomin stevedomin Adds support for optionalcallback and optionalmacrocallback
The optionalcallback or optionalmacrocallback attribute also defines the
Erlang optional_callbacks attribute.

If a (macro)callback has been defined as optional(macro)callback and
(macro)callback we emit a warning and defines the optional_callbacks
attribute.

http://www.erlang.org/doc/design_principles/spec_proc.html#id74952
b301f43
@stevedomin
Contributor

Technically @fishcakez did a lot of the work

@josevalim
Member

Thank you @stevedomin!

I would go with the names @optional_callback and @optional_macrocallback. However, I am wondering if it makes more sense to go with the Erlang style and just have @optional_callback [foo: 1, bar: 2].

Thoughts? /cc @fishcakez

@fishcakez
Member

@josevalim I thought this style was much more in line with elixir than the
Erlang style.

On Thursday, 3 December 2015, José Valim notifications@github.com wrote:

Thank you @stevedomin https://github.com/stevedomin!

I would go with the names @optional_callback and @optional_macrocallback.
However, I am wondering if it makes more sense to go with the Erlang style
and just have @optional_callback [foo: 1, bar: 2].

Thoughts? /cc @fishcakez https://github.com/fishcakez


Reply to this email directly or view it on GitHub
#4037 (comment).

@josevalim
Member

I thought this style

Which style? The @optional_callback [foo: 1, bar: 2] one?

@fishcakez
Member

Sorry the style in the PR is more elixir style. We don't use two attributes
or defines when it can be done in one.

On Thursday, 3 December 2015, José Valim notifications@github.com wrote:

I thought this style

Which style? The @optional_callback [foo: 1, bar: 2] one?


Reply to this email directly or view it on GitHub
#4037 (comment).

@josevalim
Member

@fishcakez yes, I know. But optional_macrocallback is too long. :( And we do have a couple things that mimic how optional behaves, for example defoverridable, import and @compile :inline.

@lexmag
Member
lexmag commented Dec 3, 2015

👍 for underscore after optional in every occurrence. :bowtie:

@fishcakez
Member

Hmm. An optional callback is different to those examples because they add
extras features to something else. Whereas the optional callback as a whole
is the extra feature. If reading the source code its possible to miss that
a callback is not required if its defined in multiple places. This is
similar to def/defp and type/typep. For inlining and overridable it is less
the case as by default you do nothing.

Optional_macrocallback is not so nice.

On Thursday, 3 December 2015, Aleksei Magusev <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

[image: 👍] for underscore after optional in every occurrence. [image:
:bowtie:]


Reply to this email directly or view it on GitHub
#4037 (comment).

@stevedomin
Contributor

I don't have strong opinions on naming (and not enough context to make the decision) so once you guys have settled on something I'll update my PR.

I'll also try to change the behaviour according depending on what you decide.

@beedub beedub referenced this pull request in kafkaex/kafka_ex Dec 4, 2015
Closed

CI? #56

@fishcakez
Member

optional_macrocallback really is a pain to use both because of its unwieldy length and actually using an optional macro callback. I tried to create some example use cases but I couldn't really come up with anything that was both clean and useful :(.

@lexmag
Member
lexmag commented Dec 7, 2015

Choosing between to underscore optional or not I still like underscored version. :bowtie:
My reasoning is that it assists in reading and more clearly shows a group of optional callbacks with multiply definitions.

@fishcakez
Member

@lexmag, to be clear I was not suggesting that _ was one too many characters. Certainly having it would be better. This PR does not use the same attribute style as Erlang and which pattern we should use:

 @optional_macrocallback add(number, number) :: Macro.t,

Versus

@macrocallback add(number, number) :: Macro.t
@optional_macrocallback add: 2

@stevedomin if you are up for it, would you like to try a separate branch that uses the later pattern? This does indeed fit better with defoverridable and means optional callback definitions do not need to start so far over the page that it nearly always end up requiring multiple lines. This method has the benefit that it is less intrusive to the compiler.

@ericmj
Member
ericmj commented Dec 7, 2015

Isn't including callback in the name redundant when we already have optional? What about the names @optional foo() :: bar and @macrooptional foo() :: bar. I admit it's not perfect but I think it's better than what we've seen so far.

@stevedomin
Contributor

@ericmj this might be only me but I tend to like descriptive name and optional feels a bit too generic in that sense. For a newcomer to the language it might not be obvious what optional means when just reading code whereas optional_callback is fairly explicit. Although you could argue that they wouldn't necessarily know what a callback is in the first place.

@fishcakez ok I'll work on this

@josevalim
Member

@stevedomin to clarify, we don't need both @optional_callbacks and @optional_macrocallbacks, we will have a single @optional_callbacks that will receive a keyword list of function/arity that may be both callback or macro callbacks.

@stevedomin
Contributor

I'm sorry guys I have been really busy at work this week, I will hopefully have a go at this over the week-end

@josevalim
Member

No worries at all @stevedomin. Take your time. :)

@josevalim
Member

Revamping this because we may need it for GenRouter/GenStage. We have three options:

  1. Use @optional_callback and @optional_macrocallback
  2. Use @callback and @macrocallback with an @optional_callbacks foo: 1, bar: 2 call
  3. Use @callback and @macrocallback with an @optional_callback true before every annotation

I would say 3 would be too foreign for Elixir. 1 is too long and uncomfortable. I am 👍 for 2 because it mimics defoverridable, @compile and others. The implementation for 2 is also going to be much simpler. It would be mostly a matter of marking the attribute as registered and to accumulate.

Can we agree on 2?

@whatyouhide
Member

@josevalim would we have separate @optional_callbacks and @optional_macrocallbacks or would @optional_callbacks cover both functions as well as macros?

@fishcakez
Member

@whatyouhide just one attribute

I think @stevedomin has some work on this approach already. It uses the same methods to accumulate as @spec/@type and then rewrites the macrocallbacks to the erlang function name/arity.

@whatyouhide
Member

Awesome. I'm 👍 for number 2 as well, as it's inline with what Erlang does as well :).

@lexmag
Member
lexmag commented Mar 31, 2016

2. 👏

@lpil
Contributor
lpil commented Apr 4, 2016

2 seems nice :)

@josevalim
Member

@stevedomin could you please send a new pull request that adds 2. as discussed above? If not, no problem, let us know so someone else can pick up the issue. Thank you! :)

@stevedomin
Contributor

Sorry 😔

Having another go at it this week-end

@lpil
Contributor
lpil commented May 14, 2016

Thank you Steve!

@josevalim josevalim modified the milestone: v1.3.0 May 17, 2016
@whatyouhide
Member

@josevalim I think we can close this while @stevedomin is working on the new PR?

@josevalim
Member

It is up to him. He may still force push to this branch. :)

@whatyouhide
Member

Ah, alright! Sorry for the false alarm then :)

@josevalim
Member

@stevedomin since you are likely busy. Are you ok with someone else tackling this? :)

@stevedomin
Contributor

Yes, go for it, made some progress but not enough

@josevalim
Member

Thanks for pinging back @stevedomin! :)

@josevalim josevalim closed this May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment