Add addon filter objects #2136
Add addon filter objects #2136
Conversation
|
I added the @jaredlockhart review and then removed it because he was doing a trial of his slackbot and wanted to see if it was working :) |
normandy/recipes/filters.py
Outdated
| if any_or_all == "all": | ||
| return "&&".join( | ||
| (f'normandy.addons["{addon}"].isActive' for addon in self.initial_data["addons"]) | ||
| ) | ||
| elif any_or_all == "any": | ||
| return "||".join( | ||
| (f'normandy.addons["{addon}"].isActive' for addon in self.initial_data["addons"]) | ||
| ) | ||
| else: | ||
| raise serializers.ValidationError( | ||
| f"Unrecognized string for any_or_all: {any_or_all!r}" | ||
| ) |
There was a problem hiding this comment.
probably a cleaner way to do this is:
if any_or_all == "all":
glue = "&&"
elif any_or_all == "any":
glue = "||"
else:
raise # Exception here
return glue.join(f'...')There was a problem hiding this comment.
yes you're right.
There was a problem hiding this comment.
Not to butt in but generally for this kind of mapping pattern you can use a dictionary
There was a problem hiding this comment.
wondering if we will really gain much here trying to have these two share code
I think in general anytime you are doing the same thing (with the same underlying motivation) this is code that can be factored out to keep things DRYer. The benefit is maintenance is easier.
this is far from complete code, but you could have something in this shape:
class BaseAddonFilter(BaseFilter):
# all the usual stuff goes here
def get_formatted_string(self, addon):
raise NotImpementedError("Error message.")
def to_jexl(self):
# all the stuff to determine glue
return glue.join(self.get_formatted_string(addon) for addon in self.initial_data["addons"])
class AddonInstalledFilter(BaseAddonFilter):
def get_formatted_string(self, addon):
return f'normandy.addons["{addon}"]'
class AddonActiveFilter(BaseAddonFilter):
def get_formatted_string(self, addon):
return f'normandy.addons["{addon}"].isActive'
| def create_basic_filter(self, addons=["@abcdef", "ghijk@lmnop"], any_or_all="any"): | ||
| return AddonInstalledFilter.create(addons=addons, any_or_all=any_or_all) | ||
|
|
||
| def test_generates_jexl_installed_not_active(self): |
There was a problem hiding this comment.
I the the names of these tests are out of date? It looks like they may have been written before the filter was split into two.
2136: Add addon filter objects r=rehandalal a=uhlissuh fixes #2104 So, splitting these into two filter objects came from Rehan, which I thought was a good idea. He mentioned seeing about having them share some code, because there is duplication here, and I kinda looked at that for a while and arrived at wondering if we will really gain much here trying to have these two share code? There's only two that are shaped like this right now. There isn't a clear thing I could pull out, maybe someone sees something. Perhaps that's an argument for making them one filter? Co-authored-by: Alissa Sobo <[email protected]>
Canceled |
|
bors merge |
fixes #2104
So, splitting these into two filter objects came from Rehan, which I thought was a good idea. He mentioned seeing about having them share some code, because there is duplication here, and I kinda looked at that for a while and arrived at wondering if we will really gain much here trying to have these two share code? There's only two that are shaped like this right now. There isn't a clear thing I could pull out, maybe someone sees something. Perhaps that's an argument for making them one filter?