ReviewBot: consume the generator when using search_review(), and ensure the behaviour between platform is consistent#3302
Conversation
|
Alternatively, since the only (known) user is ReviewBot.py, we can force a list there. |
Since we switched to a generator, let's consume it and store it as a list. Signed-off-by: Eugenio Paolantonio <eugenio.paolantonio@suse.com>
This follows what has been done already for the Gitea platform, to ensure behaviour consistency between the two. Users are now expected to always get a generator. Signed-off-by: Eugenio Paolantonio <eugenio.paolantonio@suse.com>
Signed-off-by: Eugenio Paolantonio <eugenio.paolantonio@suse.com>
g7
force-pushed
the
epaolantonio/gitea-plat-search-regression
branch
from
959e3e2 to
28b165e
Compare
|
v2: as agreed in Slack - let's turn it the other way: turn obs's search_platform() a generator too, and ensure that ReviewBot consumes it and stores it as a list. |
g7
changed the title
|
|
||
| def set_request_ids_search_review(self): | ||
| self.requests = self.platform.search_review(review_user=self.review_user, review_group=self.review_group) | ||
| self.requests = list(self.platform.search_review(review_user=self.review_user, review_group=self.review_group)) |
There was a problem hiding this comment.
That's reasonable, but is there really a hard dependency for self.requests being a list at the moment tho? If not I think maybe we can take this chance to change this to a generator and offer a chance for a small optimization. What do you think?
There was a problem hiding this comment.
I think it would be nice, just worried about the usage elsewhere. From a rapid skimming through ReviewBot.py:
- ReviewBot.ids_project should be moved to a generator: https://github.com/g7/openSUSE-release-tools/blob/28b165e775a5371ee3a866058b74907293219696/ReviewBot.py#L697
- ReviewBot.set_requests_id should be moved too: https://github.com/g7/openSUSE-release-tools/blob/28b165e775a5371ee3a866058b74907293219696/ReviewBot.py#L236
- ReviewBot.set_request_ids_search_review should be moved as well (this line)
Doesn't look that bad to be honest, but perhaps that can be done later in another PR, what do you think?
There was a problem hiding this comment.
A follow up pull request later sounds good. Thanks for the investigation!
|
LGTM |
2 participants
Users of search_review() expect a list, not a generator.