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

Added listener remove functions #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Grohden
Copy link
Contributor

@Grohden Grohden commented Dec 10, 2018

This should close #48
I added two listener removal functions.. but i think we need to discuss some situations

  • If the task is being 'listened' at the time of the removal.. what should happen?
  • We should really remove all instances when they're the same?
  • Should we use predicates instead of instances as param? (i personally prefer predicates, the dev has more control over it)
  • Is there any thread restriction to remove a listener?

Also, we need some tests on Wendy and examples on the example app..
I think that testing needs to be done after #50 ... and the example app i don't have ideas for the remove functions uses

@Grohden
Copy link
Contributor Author

Grohden commented Dec 10, 2018

I need to mention that dokka generates the .html files based on the system path separator.. which is bad, i got 99 files of diff after generating the docs on windows (my fedora is broken :/), is there any config for that?

@levibostian
Copy link
Owner

Replaying to your comment @Grohden

If the task is being 'listened' at the time of the removal.. what should happen?

What do you mean by "task is being listened"? If a task is currently added as an active listener?

We should really remove all instances when they're the same?

Yes

Should we use predicates instead of instances as param? (i personally prefer predicates, the dev has more control over it)

Predicates are great, but in this case, not required, right? Listeners will be object instances so we can run .equals() to see if the object instances are the same reference. That should do the job.

Is there any thread restriction to remove a listener?

WendyConfig is not thread-safe at this time, as you can see. We will need to make the list of listeners and the mutating functions all thread safe. It would be difficult to see if adding thread safety code works until we have unit testing setup.

With that in mind, I am leaning towards waiting to merge this PR (since it's a nice-to-have and not required since we are using weak references) until we have unit testing setup and this PR can be fully tested. Agree?

Also, we need some tests on Wendy and examples on the example app..

Agree. That should be the top priority at this time.

the example app i don't have ideas for the remove functions uses

The example app used to exist for me to test that Wendy works while not yet having unit testing setup. Because unit testing is a high priority now, the example app will exist to show off best practices and give some example code that people can use to play around with the library. With that in mind, I do not believe we need to add functionality to the example app to remove listeners.

@levibostian
Copy link
Owner

Thanks for the PRs, @Grohden! You are helping to make Wendy better and better.

@Grohden
Copy link
Contributor Author

Grohden commented Dec 10, 2018

@levibostian

What do you mean by "task is being listened"? If a task is currently added as an active listener?

I mean, if you write a listener that removes itself on one of its callbacks. (i do not expect someone to do this.. but ¯\(ツ)/¯)

With that in mind, I am leaning towards waiting to merge this PR (since it's a nice-to-have and not required since we are using weak references) until we have unit testing setup and this PR can be fully tested. Agree?

Agreed, i'm not in a hurry for this

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.

Provide a way to remove listeners
2 participants