Code review comment for lp://qastaging/~johannes.erdfelt/nova/deferred-delete-instance

Revision history for this message
Johannes Erdfelt (johannes.erdfelt) wrote :

> Looks good. I like the delete filtering implementation to solve the deleted
> instances showing up problem. You could actually just do:
>
> if filters.get('deleted', False):
> .. code for True
> else:
> .. code for False

The check to see if it's been defined is necessary to match the current behavior.

> Removing 'deleted' from the list of direct match names causes it to be ignored
> below this code IIRC, so you don't really need to pop it out of the dict.
> Even if you wanted to pop it out, you could s/get/pop/ above.

The code further down was resulting in an extra check for deleted if I didn't remove it from the filters dict. Other code (like changes-since) didn't need to remove it because the name doesn't match a column. _regexp_filter_by_column will return True in that case.

The code is a little messy there as it is. I'm not sure why regexp_filter_funcs is still there for instance. It's checked, but no keys are ever set, so it's effectively a noop. Plus the aforementioned return True seems like a hack.

« Back to merge proposal