Closed jc0n closed 12 years ago
Please add a comment to the docblock for FlatMerge.call and NestedMerge.call about what the "result" parameter should be. I am currently trying to bug-fix and am having to read backwards through your code to figure this out.
For the future, all methods should have parameters documented, unless it's something very common like "auth_token", or is a very standard method definition in the API.
I'll update the documentation.
For now the quick explanation is that those Merge classes are somewhat of a strategy pattern. The call defines the implementation and it is applied to a result (a list of dictionaries as returned by Getter) here https://github.com/TireSwingSoftware/openassign-server/commit/98515addade85b331bd0dabb24d712dfc09a1d33#L1R961. The initial result is https://github.com/TireSwingSoftware/openassign-server/commit/98515addade85b331bd0dabb24d712dfc09a1d33#L1R956. However, it changes with each merge.
Ok. It looks like there are lots of important methods that are missing documentation, like View._parse_foreign_key
. View._raw_query
seems important, but there's no documentation at all. There are many others.
Please review all the code you wrote related to this task, and add method documentation where missing. Each method should have:
@mhrivnak: You can sign off on this one. It LGTM.
@mhrivnak: New revision is up. You may want to take a look.
Thanks! Feel free to close this issue if you're done with it.
@mhrivnak: We discussed this via IM recently so I won't provide too much detail here. This is for the sake of tracking the change.
Most of the views are all the same in terms of the wrapping get_filtered and performing a few simple (potentially nested) merges. I have created a generic ViewBuilder object which allows this work to be encapsulated elsewhere. It provides a couple of benefits:
A demonstration of the ViewBuilder semantics applied to the AssignmentManager.transcript_view, one of the more complicated in terms of required nested merges: