-
Notifications
You must be signed in to change notification settings - Fork 1
NameTransformation class-level attribute #18
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
NameTransformation class-level attribute #18
Conversation
|
@sad-spirit Yo! I've reviewed your code which is fine, but I'd like to discuss things a bit if that's ok with you. Also, not a fan of the method name Otherwise nice job and thanks for contributing. Sorry again for the delay in my answers. I reactivated email notifications, I should be more responsive starting now. |
Maybe So it will look like
You are right, I could choose a better name. Same as above, I'd suggest
👍 |
|
Maybe |
The attribute does not perform any actions, it just specifies which actions to perform. I'll update the pull request (and fix the CI failures). |
|
I think it's 99% mergeable, nicely done. Do you think you could test |
That's mostly done already: note that |
|
It sort of is... Let's merge this and I'll add more tests whenever (to cover also the error that happens when two Identifier attributes are used). Thanks again! |
|
Thanks! What I actually forgot is a test for |
|
Ha yes the overriding was mentioned in the docs and I forgot to tell you about it. Maybe make a small pr when you have the time, no worries. Coverage also took a small hit https://app.codecov.io/gh/Pixelshaped/flat-mapper-bundle/commit/7c8f66c683a6979b36a0c2e5d373c7e363092ff5 but it shouldn't be too hard to cover that back. |
This implements the new class-level
#[NamePrefix]and#[Camelize]attributes as described in request #17Additionally, having more than one
#[Identifier]attribute on DTO class level will cause an exception, as was probably intended.Fixes #17