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

Hide Channel internal implementation methods #9564

Conversation

j8r
Copy link
Contributor

@j8r j8r commented Jul 1, 2020

Some methods should not be part of the public API, being internal implementation details.
Obviously, receive_impl and receive_internal are ones.

Return type annotations are also added for receive and receive? to have them in the API docs.

@waj
Copy link
Member

waj commented Jul 2, 2020

Thanks! Channel's interface and docs is needing some love. There are other methods I'd like to hide from docs. select_impl looks obvious, but I think we could hide anything related to SelectAction as well. @bcardiff what do you think? Methods to create instances of SelectAction are not even public anyway and it's an implementation detail right now. Other structs like SelectState and DeliveryState should be marked as :nodoc: as well.

@j8r would you like to make those updates as part of this PR?

@j8r j8r force-pushed the hide-channel-internal-implementation-methods branch from d701da2 to 37b055b Compare July 2, 2020 16:29
@j8r
Copy link
Contributor Author

j8r commented Jul 2, 2020

What about methods receiving SelectAction or Indexabel(SelectAction) as an argument?

@waj
Copy link
Member

waj commented Jul 2, 2020

I think those should be :nodoc: as well. Those methods are called when a select statement is used.

@j8r
Copy link
Contributor Author

j8r commented Jul 2, 2020

I agree with you @waj , and it will definitely be good to to enhance Channel API before 1.0.0.

Also some methods/classes can be changed from :nodoc: to private - which is better.

@j8r j8r force-pushed the hide-channel-internal-implementation-methods branch from 76aaaf6 to 657338d Compare July 2, 2020 20:41
@bcardiff
Copy link
Member

bcardiff commented Jul 2, 2020

I agree that the self.non_blocking_select/self.select can be made :nodoc:. They are used by the compiler so private will not work on them. But for the user the api should be to use the select statement.

We are leaving doing send + receive on array as a public API but I think it's safe to do so.

I don't follow why you are changing Channel to AChannel though.

@j8r
Copy link
Contributor Author

j8r commented Jul 2, 2020

Sorry @bcardiff , I was testing something and pushed accidentally.

@j8r j8r force-pushed the hide-channel-internal-implementation-methods branch 2 times, most recently from 129d473 to 7a9db65 Compare July 2, 2020 20:55
@waj
Copy link
Member

waj commented Jul 3, 2020

@j8r this looks great. Could you please run crystal tool format?

Also, def self.select(ops : Indexable(SelectAction), has_else) can be removed now. It was deprecated for some versions but it's not needed anymore and it was always internal anyway.

@j8r j8r force-pushed the hide-channel-internal-implementation-methods branch from 79dca90 to 02a5533 Compare July 3, 2020 14:51
@waj waj added this to the 1.0.0 milestone Jul 6, 2020
@straight-shoota
Copy link
Member

Thanks @j8r

@j8r j8r deleted the hide-channel-internal-implementation-methods branch July 9, 2020 10:18
asterite pushed a commit to asterite/crystal-1 that referenced this pull request Jul 18, 2020
* Hide Channel internal implementation methods
* Add private/nodoc to more methods and types
* Remove deprecated Channel#select
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants