Bucky789 commented on code in PR #60963:
URL: https://github.com/apache/airflow/pull/60963#discussion_r2733486701
##########
providers/imap/src/airflow/providers/imap/hooks/imap.py:
##########
@@ -240,12 +260,17 @@ def _retrieve_mails_attachments_by_name(
return all_matching_attachments
- def _list_mail_ids_desc(self, mail_filter: str) -> Iterable[str]:
+ def _list_mail_ids_desc(self, mail_filter: str, max_mails: int | None =
None) -> Iterable[str]:
if not self.mail_client:
raise RuntimeError("The 'mail_client' should be initialized
before!")
_, data = self.mail_client.search(None, mail_filter)
mail_ids = data[0].split()
- return reversed(mail_ids)
+ mail_ids_desc = reversed(mail_ids)
Review Comment:
Thanks both for the insights
I see @uranusjr’s point about intent and maintainability: since max_mails is
meant as a performance and scoping feature, taking the last N IDs first and
then reversing them makes the logic more explicit (“latest N, processed
newest-first”) and avoids conceptual overhead.
At the same time, I also understand @vaefremov95’s clarification that
reversed() combined with itertools.islice() is lazy and does not allocate a new
list or reorder data in memory, unlike slicing. From a purely technical
standpoint, that approach is valid and efficient.
Given Airflow’s emphasis on readability and long-term maintainability, I’m
happy to align with whichever direction you prefer. If slicing-first is the
preferred approach, I can update the logic along these lines:
```
_, data = self.mail_client.search(None, mail_filter)
mail_ids = data[0].split()
if max_mails is not None:
mail_ids = mail_ids[-max_mails:]
return reversed(mail_ids)
```
This keeps the behavior clear (“take the latest N, then process
newest-first”) and avoids islice() altogether.
Please let me know which approach you’d like me to adopt, and I’ll update
the implementation accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
