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 __filterChanged to filterChanged #336

Open
dotpointer opened this issue Jan 27, 2020 · 5 comments
Open

Rename __filterChanged to filterChanged #336

dotpointer opened this issue Jan 27, 2020 · 5 comments

Comments

@dotpointer
Copy link
Contributor

dotpointer commented Jan 27, 2020

By design __filterChanged needs to be overridden, but it needs to be renamed to filterChanged because it is private.

@dotpointer dotpointer changed the title Rename _filterChanged to filterChanged Rename __filterChanged to filterChanged Jan 27, 2020
@cristinecula
Copy link
Collaborator

Can you give me an example where you would override __filterChanged?

@dotpointer
Copy link
Contributor Author

dotpointer commented Jan 27, 2020

Can you give me an example where you would override __filterChanged?

This issue was requested by @nomego so he knows more, but the problem appears in cosmoz-omnitable-column-boolean.js when running yarn lint, then it prints:

cosmoz-omnitable-column.js(84,2) warning [overriding-private] - Overriding private member '__filterChanged' inherited from columnMixin

The renamings of ( _ [ _ ])filterChanged should be done all over cosmoz-omnitable as told by nomego.

@cristinecula
Copy link
Collaborator

@nomego Looking at the code, it becomes apparent that the purpose of columnMixin's __filterChanged is to notify the cosmoz-column-filter-changed event. Looking at the usage in cosmoz-omnitable-column, the method is overridden to add a behavior unrelated to the original purpose:

https://github.com/Neovici/cosmoz-omnitable/blob/master/cosmoz-omnitable-column.js#L84-L89

I'm not sure if making the private method public is the best solution. Maybe it would be better to keep it private and instead set up a different observer in cosmoz-omnitable-column, just like in cosmoz-omnitable-column-boolean:

https://github.com/Neovici/cosmoz-omnitable/blob/master/cosmoz-omnitable-column-boolean.js#L90-L118

@nomego
Copy link
Contributor

nomego commented Jan 27, 2020

So now Boolean is broken cause it doesn't fire the event? (Haven't looked at the code)

I haven't actually analyzed the logic at all, just gave the answer to solve the warning.

If the nature of the observer is internal and not meant to be overridden we should change Boolean to not override it.

@cristinecula
Copy link
Collaborator

@nomego no, boolean has both __filterChanged (inherited from mixin) and _filterChanged (own method, notice the single underscore).

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

No branches or pull requests

3 participants