-
Notifications
You must be signed in to change notification settings - Fork 1
refactor! Rename operation classes & methods to xxxHandler
#13
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
Conversation
|
Since we are going for a new major version, I agree with renaming all relevant entities to maintain consistency. Breaking changes can be expected by end-users, but I will make sure to explain in the release notes what has changed and how to upgrade. I'm also ok with splitting the task across two separate PRs. Thanks for your further analysis and improvements 💪 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
6afe21b to
7bbfffe
Compare
|
@blancks apologies - while working on the other PR I noticed a couple of nits that had crept into this PR (I was on a laptop this morning rather than my usual dev machine). I've force pushed to the branch to fix those. |
Adds a `Handler` suffix to all current final operation handler class names, and moves them to a `handlers` namespace. This is not expected to affect end-users as the in-built handler classes are not generally expected to be used outside the package. This will avoid naming conflicts and confusion when we subsequently introduce support for representing a patch as a set of operation DTOs.
End-users are not expected to reference these classes directly. If users want to customise handling for a particular operation, they should implement the interface and/or extend from the abstract base class.
Renames all remaining classes, interfaces & methods related to patch operation handlers to include the `Handler` suffix. This reinstates consistency between the naming of these concerns, and will provide the cleanest base for implementing patch operation DTOs without naming conflicts or confusion. It will, however, impact any end-users who have implemented & registered custom patch handlers as they will need to update them for the new naming as part of the 3.x upgrade.
7bbfffe to
e8b5a9f
Compare
| use blancks\JsonPatch\json\handlers\JsonHandlerAwareTrait; | ||
| use blancks\JsonPatch\json\pointer\JsonPointerHandlerAwareInterface; | ||
| use blancks\JsonPatch\json\pointer\JsonPointerHandlerAwareTrait; | ||
| use blancks\JsonPatch\operations\handlers\PatchOperationHandlerInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are in the same namespace, this can be removed safely if you agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right - PHPStorm added it as I moved the classes one at a time, and I didn't notice this one was still in there. Fixed now.
This is a preparatory refactor for 3.0, ahead of implementing #12
We had so far only discussed renaming the concrete classes e.g.
Add =>AddHandler`, on the basis this should not affect end-users except in unusual cases.Since we are now targeting this for a new major version, I have gone further than discussed and renamed all the classes, interfaces and methods related to patch operation handlers to include the
xxxHandlersuffix and moved them to anoperations/handlersnamespace.I think this is best for consistency within the codebase, and avoiding any potential naming conflicts / confusion from #12.
However, it comes at the cost that any end-users who have implemented / registered custom handlers will need to do a little renaming before upgrading to 3.x. I'm not sure how common that's likely to be, and it should be an easy change to apply, but worth considering.
I've therefore split this into separate commits so I can easily limit to just renaming the concrete classes if preferred.