Conversation
prijendev
changed the title
| else: | ||
| raise RuntimeError("Unable to retrieve timezone information from the Mambu endpoint. Please provide administrator credentials or configure valid timezone in the UI(e.g., US/Pacific)." \ | ||
| " Refer this for more details: https://support.mambu.com/docs/organization-contact-currency-and-timezone") |
There was a problem hiding this comment.
It's too wide, let's follow PEP 8's 79-character line limit
| else: | |
| raise RuntimeError("Unable to retrieve timezone information from the Mambu endpoint. Please provide administrator credentials or configure valid timezone in the UI(e.g., US/Pacific)." \ | |
| " Refer this for more details: https://support.mambu.com/docs/organization-contact-currency-and-timezone") | |
| raise RuntimeError( | |
| "Unable to retrieve timezone information from the Mambu endpoint. " | |
| "Please provide administrator credentials or configure valid timezone " | |
| "in the UI(e.g., US/Pacific). Refer this for more details: " | |
| "https://support.mambu.com/docs/organization-contact-currency-and-timezone" | |
| ) |
| if self.has_more or self.start_datetime < self.end_datetime: | ||
| raise StopIteration() |
There was a problem hiding this comment.
This condition looks to me inverted. Shouldn't it raise when there's NO more data?
| def _all_fetch_batch_steps(self): | ||
| # Large buffer size can impact memory utilization of connector | ||
| # so empty the buffer once it reaches default max limit | ||
| if len(self.buffer) > self.max_buffer_size: | ||
| return |
There was a problem hiding this comment.
This makes me still think of potential memory issues - buffer management logic may still accumulate large amounts of data because the check happens after fetching, not before. (just thinking out loud)
agzam
left a comment
agzam
left a comment
There was a problem hiding this comment.
I like good error messages - clear runtime errors. Backward compatibility looks good. Good debug logging. Overall - great work!
The 5-minute overlap strategy is confusing, consider adding some explanation in comments.
I don't like that the PR explicitly adds untested code path due to missing API keys, but that's what we have to roll with sometimes (not my call to make)
I really don't want to block this work - thus only comments for now. Will gladly give it another gander sometime later.
3 participants
Description of change
timezonewhich we would use in case if customer has not provided admin creds.Manual QA steps
Risks
Rollback steps
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code