Fix bugs in required and excluded routes #18
Conversation
| 'target': 'tests.D', | ||
| 'source': 'tests.B' | ||
| }, | ||
| # { |
There was a problem hiding this comment.
Remove these commented lines unless they have some purpose I'm missing.
|
@naegelyd Thanks for the comments. Let me know when you are done with comments and I will update. |
| # Model level.. | ||
| elif source == self._excluded_joins.get(target): | ||
| return False | ||
| # Definition of required join: for the specified target, |
There was a problem hiding this comment.
I think this comment can be removed. It doesn't directly describe any code and I think required join is fairly obvious since the idea of a required route is well described in other comments above.
It is now an error to specify more than one required route with the same target (ValueError is raised). It is now possible to define multiple excluded routes involving the same target model. Previously, the last defined exclusion route for a target model would silently replace any other such routes. Signed-off-by: Kevin Murphy <murphyke@email.chop.edu>
| # \ / \ | | ||
| # J H | ||
| # | | | ||
| # K I |
There was a problem hiding this comment.
Look at this lovely ascii art.
Fix bugs in required and excluded routes
|
I am running into an issue when using the 'required_routes': [{
'source': 'production.audiogramtest',
'target': 'production.audiogramresult',
'field': 'audiogramresult.test',
'symmetrical': True,
}, {
'source': 'production.encounter',
'target': 'production.audiogramtest',
}, ...]The first route is conditional on a join occurring between Of course this distinction of condition vs. declaration is not implemented. It is only the latter. I am not suggesting this is a bug in this PR, but it uncovered the distinction. However, the @murphyke One unrelated question I would like to ask is why did you choose to replace the |
|
@bruth I changed the data structures to allow defining multiple excluded routes involving the same target model. In the PR comment I alluded to the fact that the lookup was now less efficient. I guessed that this wouldn't matter since the modeltrees would in general be "small" and wouldn't be built very often. It would also be possible to continue keying on target but have the dict values be lists instead of single values. As the keys became (source, target) tuples, it seemed to simplify things to store the optional field as the dict value. It was partly that simplification that caused me to make that choice. |
|
@murphyke Understood and thanks. I pushed the fix for checking for redundant targets when |
It is now an error to specify more than one required route with the
same target (ValueError is raised).
It is now possible to define multiple excluded routes involving the
same target model. Previously, the last defined exclusion route for
a target model would silently replace any other such routes.
The ModelTree
_required_routesand_excluded_routesfields, whichused to be dicts keyed on target model, are now dicts keyed on
(source, target) tuple, and the value of the dict is the join field, or None
if there is no join field specified.
_required_join_fieldsand_excluded_join_fieldsare no longer needed. The code is simpler, butlookup of a target in the excluded routes dict is less efficient now.
I expect that doesn't matter much, but if it does, we can change
to keying on target, with the value being a list of source:field tuples.
Signed-off-by: Kevin Murphy murphyke@email.chop.edu