Skip to content
This repository was archived by the owner on Feb 11, 2022. It is now read-only.

TextWatcher simple implementation #61

Merged
merged 2 commits into from
Jul 14, 2015
Merged

Conversation

danybony
Copy link
Contributor

I don't really care about beforeTextChanged or onTextChanged.
I only need the good old afterTextChanged!

-- An Android developer

@stefanhoth
Copy link
Contributor

I'd say this addition is of questionable value. While it surely moves the no-op impls to Notils, making the code seem cleaner it might add some confusion for devs who know the interface, wondering where the rest of the events go.

I think the benefit of having this in Notils is very marginal.

@blundell
Copy link
Contributor

but tbf everytime I use this interface I only want one of the 3 methods so 👍 from me

@danybony
Copy link
Contributor Author

@stefanhoth this is open to discussion.
It is a thing I saw in several projects and I always though it could have been useful to have in Notils.
Regarding the confusion for devs who know the interface I'd say that this is way less confusing than our logger implementation compared to the android.util one.

@malmstein
Copy link

👍 from me as well. Death to no-ops!

@friedger
Copy link
Contributor

❌ needs an update of the readme

@danybony The argument that there are other confusing APIs does not count :-)

Happy with it as proguard removes the class if not needed.

@danybony
Copy link
Contributor Author

@friedger readme updated!

@friedger
Copy link
Contributor

👍 Letting @stefanhoth merge it

@stefanhoth
Copy link
Contributor

I wouldn't X it but I still don't see the big benefit and would rather have this No-op base class in my own project. But if @malmstein or @blundell are big fans they should merge it.

blundell added a commit that referenced this pull request Jul 14, 2015
@blundell blundell merged commit 9f055cd into master Jul 14, 2015
@blundell blundell deleted the textwatcher_implementation branch July 14, 2015 12:09
@blundell
Copy link
Contributor

👼

@malmstein
Copy link

💃

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

Successfully merging this pull request may close these issues.

5 participants