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

Rename promote_rule -> Promote.rule #23999

Closed
timholy opened this issue Oct 5, 2017 · 7 comments
Closed

Rename promote_rule -> Promote.rule #23999

timholy opened this issue Oct 5, 2017 · 7 comments
Labels
needs decision A decision on this change is needed
Milestone

Comments

@timholy
Copy link
Member

timholy commented Oct 5, 2017

I am pretty sure that promote_rule and promote_type derive from an era before we had modules. Following up on #23939 (comment), the suggestion is to not use "namespacing by underscoring" and just use namespacing directly. promote.jl could define a Promote module and export promote but not rule, so that users would write Promote.rule. (#23939 is planning to introduce Broadcast.rule.)

There's one huge hitch with this suggestion: promote_type should become Promote.type, and you can't define a function called type. We could call it Promote.typejoin and have it fall back to Base.typejoin.

@timholy timholy added the needs decision A decision on this change is needed label Oct 5, 2017
@timholy timholy added this to the 1.0 milestone Oct 5, 2017
@fredrikekre
Copy link
Member

you can't define a function called type

Should be possible for 1.0 though?

@fredrikekre fredrikekre added the triage This should be discussed on a triage call label Oct 5, 2017
@timholy
Copy link
Member Author

timholy commented Oct 5, 2017

But if we can't use it for 0.7, can we use it for 1.0?

@mschauer
Copy link
Contributor

mschauer commented Oct 5, 2017

promote_type promotes types, so that is perhaps not a case of "namespacing by underscoring".

@nalimilan
Copy link
Member

For a little bit more context, at #20815 I propose another promotion mechanism for array concatenation, which could be called cat_rule or something like that. So it would make sense to unify the naming of the three promotion systems.

However, Promote.type looks really weird. Maybe better stick with the current names. The main reason for this suggestion is that the Broadcast module would provide rule, similar and indices, but maybe we can find another solution.

@JeffBezanson
Copy link
Member

It doesn't fully make sense to me that Promote would be a module.

@timholy
Copy link
Member Author

timholy commented Oct 5, 2017

So should we go with namespacing by underscore then, and have Broadcast export broadcast_rule, broadcast_indices, and broadcast_similar to Base? I don't think it makes any sense to type

Base.Broadcast.broadcast_rule(...) = ...

but

Base.broadcast_rule(...) = ...

seems OK.

@timholy
Copy link
Member Author

timholy commented Oct 5, 2017

Seems like there's consensus, I'll close.

@timholy timholy closed this as completed Oct 5, 2017
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Oct 5, 2017
@ararslan ararslan mentioned this issue Jan 17, 2018
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

6 participants