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

implement is_empty() and use it according to Clippy's prophecy. #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

implement is_empty() and use it according to Clippy's prophecy. #98

wants to merge 1 commit into from

Conversation

igalic
Copy link
Contributor

@igalic igalic commented May 29, 2020

Clippy complains that we have a .len() method, but no corresponding .is_empty() method.
Once implemented, it then recommends using !is_empty() in place of len() > 0.

@igalic
Copy link
Contributor Author

igalic commented May 29, 2020

This now leads to:

warning: method is never used: `len`
   --> src/actor/actor_cell.rs:723:5
    |
723 |     pub fn len(&self) -> usize {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: method is never used: `len`
   --> src/actor/actor_cell.rs:723:5
    |
723 |     pub fn len(&self) -> usize {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

so unless we at least create tests and/or rustdoc comments, it can be removed?

@leenozara
Copy link
Contributor

@igalic I think if we remove len()/ we’ll find that we need it back in there again fairly soon. I believe it was used previously on an earlier impl of Context and later lost in a major refactoring.

Context is the “read-only” user view of the actor cell. We could clean up the code as you’ve done, since we don’t want dead code about, or we could look to see how to add the functionality that .len() ultimately could be used for, namely .context.children.len().

Children isn’t part of the Context type, since that is created for the current run of the actor mailbox, but it is part of ActorRef and that is available to Context.

It essentially comes down to updating the API so that user code has access to the number of children the current actor running has, via context.

So, we could remove .len() and then merge this PR in, and come back to exposing number of children in another PR. Or if doing the latter is something you might want to look at then we can hold off on this PR.

What do you think?

@igalic
Copy link
Contributor Author

igalic commented Jun 3, 2020

Great portions of this comment would make good extensions to the current documentation

I'll to to do as you say, and update the API to be clearer in its intentions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants