Hey @guidomaiolams! Thanks for the ticket, here are a few suggestions for next time:
Make sure to include a description on what this PR is doing. We do have a styleguide about this kind of stuff. Otherwise, reviewers have no clear context on what the intention is. (I'll share it here later, it seems it wasn't public.)
You did reference the ticket this solves (thanks!) but the ticket itself doesn't have much information either. Consider expanding it with details. This is specially important in open source projects like this one.
On the changes:
I noticed that several lines changed but they seem to only have whitespace changes -- consider avoiding changes that really do nothing, unless you're actually changing them on purpose.
Creating the interface so far was a good improvement, but without a base class, the behavior is still spread around across services. Maybe creating a ServiceBase class too? I think it would be a good idea.
On that design:
If you take the suggestion above, consider also moving the tests to the base class. Also, consider alternatives, maybe abstraction is not he best approach and we could have composition of services?
Services may do different things, one of them might be retrieving stuff from DB. Since these ones rely on context being available, a better name might be ContextServiceBase? This way, people extending from this project have it clear that it only applies to such cases.
Hey @guidomaiolams! Thanks for the ticket, here are a few suggestions for next time:
On the changes:
ServiceBaseclass too? I think it would be a good idea.On that design:
ContextServiceBase? This way, people extending from this project have it clear that it only applies to such cases.