Skip to content

Conversation

@thunderhook
Copy link
Contributor

Finally took the time to figure it out. I used the patch of @aboqasem mentioned in #3323 (comment) and finished the implementation. I think he should also be mentioned as co-author.

Closes #3323

@aboqasem
Copy link

Thanks a lot for the acknowledgement, @thunderhook!

I will have to take a look at it sometime to learn how it was implemented.

Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think some small changes could be done though to make the tests clearer. Let me know what you think @thunderhook

@thunderhook
Copy link
Contributor Author

Thank you! Good points. I have rewritten the tests so that the collection also has a different name.
But now the mapping seems to be generated in a different order? At least PresenceUtilsAllPropsWithSource now behaves strangely and sometimes removes the boss. prefix from the path.

I'm not sure how to solve it. I don't quite understand the logic that is implemented with those Deques.
A possible approach is to use a source name that behaves the same (makes the test less readable).
WDYT?

@thunderhook
Copy link
Contributor Author

Got it. As suspected, this problem was caused by a reordered mapping method. The PresenceUtilsAllPropsWithSource worked by accident because the nested address mapping happened at the end. The @AfterMapping after method was called by the intermediate method that handled the collection, and therefore went up the path twice.

@filiphr Could you please take another look? Thanks in advance.

@filiphr filiphr merged commit 8191c85 into main Feb 11, 2024
@filiphr filiphr deleted the 3323 branch February 11, 2024 09:42
@filiphr
Copy link
Member

filiphr commented Feb 11, 2024

Thanks @thunderhook. The changes looked good, I've merged this

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

Successfully merging this pull request may close these issues.

Add @SourcePropertyName to handle a property name of the source object

4 participants