Repository navigation
Question about categories in DiffNode #157
Description
Hi, I've succesfully usedObjectDifferBuilder to configure exclusion with categories, only the children with the specified categories are returned to the Visitorby the visitChildrenmethod of DiffNode.
I'm pretty happy with it cause it helps dealing with only parts of the object I'm diffing and merging I'm interested in.
I've noticed that calling DiffNode.getCategories() returns an empty Set, is this the expected beheaviour?
Activity
SQiShER commentedon Feb 27, 2016
Yes, but I agree that's a bit confusing. The
DiffNodeonly returns the categories discovered during introspection (from the annotation). But more can be assigned to specific nodes at runtime via configuration API. Internally theObjectDifferwill look at both, the compile time categories and the runtime categories and make a decision based on all of them.The
DiffNodedoesn't represent the whole truth, because I didn't want to introduce a dependency to the category resolver mechanism. However, I'm not opposed to the idea of having theDiffNoderepresent the actual categories. It just wasn't high enough on my todo list, to look for possible solutions. It may turn out to be very easy to do. Or it may be super hard. Who knows? 😄 If you want to give it a shot, I'd be happy to assist you on a pull request.If not, I'll probably do it myself someday. 😉
scompo commentedon Feb 28, 2016
Oh I understand, I would like to help with that, I'll provide a pull request as soon as I can.
Would exporting the code for
resolveCategoriesinCategoryServicein a strategy and use it in thegetCategoriesmethod ofDiffNodebe an ok solution in your opinion?SQiShER commentedon Feb 28, 2016
Awesome! Ok, let me give you some pointers: the basic idea of the
CategoryServicewas to move the configuration (regarding categories) into a single place. TheDiffNodeon the other side is meant to be as stupid and decoupled from the rest of the system as possible. If the code would be moved into a strategy, the node would still need to access the service to retrieve the configuration and therefore introduce an unwanted dependency.I would prefer a solution, where the service simply assigns the resolved categories as property of the node sometime during the comparison process. That way the dependency remains unidirectional and the node doesn't become much more complex than it already is.
For the public API it makes sense, that
getCategories()behaves just as you initially expected. Internally however, it may make sense to introduce different properties for the hardcoded, inherited and configured categories, depending on how you adjust the logic in theCategoryService.scompo commentedon Feb 29, 2016
Great, makes sense to keep
DiffNodedecoupled.By hardcoded you mean the ones on the annotations, right?
SQiShER commentedon Feb 29, 2016
Yup
scompo commentedon Feb 29, 2016
Thanks, just wanted to make sure I got it ;)
I didn't know anything about groovy and spock so I had to learn them, but I think I've got an integration test working.
I pushed the code I wrote this evening here: 1d15505.
I need to clean it up a bit and probably add some unit test, of course if you have any suggestion or critic I'd be glad to hear them.
SQiShER commentedon Mar 1, 2016
Sorry, I didn't get to it tonight. But I'll have a closer look at it tomorrow!
SQiShER commentedon Mar 4, 2016
Alright, I finally found the time to take a closer look and have some small suggestions. Could you open a Pull Request, so we can easily comment on specific lines of code? That would make it easier to discuss. PRs can always be amended later, so don't worry if you're not 100% done yet.
scompo commentedon Mar 4, 2016
Hi, my last days have been pretty busy too 😄
Wich branch of the main repo should I use to create the pull request from mine?
scompo commentedon Mar 4, 2016
Hope master it's fine, github went straight for it, lol.
There we go: #159