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

private methods #1321

Closed
spider-mane opened this issue Sep 27, 2020 · 3 comments
Closed

private methods #1321

spider-mane opened this issue Sep 27, 2020 · 3 comments

Comments

@spider-mane
Copy link

I've noticed a number of private methods and final classes in this project. Is there a specific reason for this? It seems like it's done as a matter of convention. I ask because I'm looking to make a contribution that will most likely require a child class using methods designated as private in a parent.

@henriquemoody
Copy link
Member

henriquemoody commented Oct 4, 2020

Hi, @spider-mane!

The reason for that is that it makes it easy to maintain the project. In general I would like to favor composition over inheritance, but it's not like it's a taboo. I've said something about that before #655 (comment)

Which class would you like to change and why? Perhaps you could send your pull request and we could talk over that.

@spider-mane
Copy link
Author

I was looking to implement a solution where a child Validatable of an AllOf instance would be able to have an independent template while still inheriting the "name" argument. It ended up being a bit more complex than I expected in part because Validator extends AllOf. Another, probably preferable option would be to create a separate class that supports this functionality. Either way, I run into significant issues resulting from the use of private methods especially if implementing the latter approach in my own project rather than submitting it as a PR to this repo. Most of the inner workings don't need to be changed, just some modifications to the getMessages method originally implemented in NestedValidationException.

Another issue arises when creating the desired implementation from scratch. The project in question needs to be able to reliably anticipate the nature of the ValidationException, i.e. whether or not it only supports a single message or multiple nested messages. I can work around this by doing a check for a getMessages() or getFullMessage(), but I think it would be better if there was an interface for exceptions that support nesting. I think this would be useful in general, but especially if the default scope of methods is to remain private.

But going back to the original issue, despite NestedValidationException being concrete, it's difficult to imagine it as not also being intended as a base class. There's a large number of classes that extend it and there's no corresponding NestedValidation implementation of Validatable. I'm not sure that I'm necessarily looking to debate the utility of private methods and final classes as a default standard, but in this particular case, it is establishing a limitation on the user that seems arbitrary and is difficult to work around. I can't really do much in the way of composition, because the behavior that I need to change is built into the class itself rather than a dependency.

@alganet
Copy link
Member

alganet commented Feb 19, 2023

For message templates, you should definitely look into:

https://github.com/Respect/Validation/blob/master/docs/message-translation.md
https://github.com/Respect/Validation/blob/master/docs/message-placeholder-conversion.md

The NestedValidationException class is not final, so you can extend it. As of the current version, both getMessages and getFullMessage are public and can be overriden.

class MyCompositeRule extends AbstractComposite
class MyCompositeException extends NestedValidationException

Those are the recipes for making rules that behave like allOf and noneOf.

Availability of custom composite rules in our Validator::|v:: façade cannot be guaranteed. So you should use the concrete API when dealing with your own composites:

https://github.com/Respect/Validation/blob/master/docs/concrete-api.md

How we deal with these groups of rules is one of the trickiest parts of the library, and there's a bunch of limitations on what we can expose for extensibility while keeping the core compatible with external rules.

This is an old issue, and you might not be working on this anymore. If you are still interested in doing so, and by following these steps you encountered a problem, feel free to reopen this. In that case, I would like you to publish a branch with WIP changes so we can also understand better what you're trying to do!

🐼

@alganet alganet closed this as completed Feb 19, 2023
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