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

Allow overriding methods or properties #66

Open
paul-thebaud opened this issue Oct 25, 2019 · 3 comments
Open

Allow overriding methods or properties #66

paul-thebaud opened this issue Oct 25, 2019 · 3 comments

Comments

@paul-thebaud
Copy link

Hi!

It would be great to have the possibility to override more methods of the tool, essentially in Model.
This will require changing their visibility from private to protected.

For example, I would like to override the get method for Models and builders where I don't have pagination, but the query of the builder is a private attribute.

@paul-thebaud paul-thebaud changed the title Allow overriding methods Allow overriding methods or properties Oct 25, 2019
@filipedtristao
Copy link
Contributor

@DavidDuwaer It's a very good idea, i think the methods and props should be all protected to allow overrinding.

Can i do a PR for this?

@DavidDuwaer
Copy link
Owner

DavidDuwaer commented Oct 28, 2019

I think this is an easy way to allow use for many more edge cases. Most importantly I think it spurs organic growth of good inventions/additions that can later be added into the library itself.

That said, these methods being overridable should not expand the API's "surface" that is required to maintain backward compatibility going onward. In other words, overriding the internals of the API by users of the library should be done at own risk. So we should state that clearly in those methods' docs. Then it's fine by me!

@paul-thebaud
Copy link
Author

@DavidDuwaer I understand.
In every case, methods which are public or protected should be more documented to understand the impacts of changing their behavior.
I leave this issue open since @filipedtristao proposed a PR.

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