Skip to content

[SAC-27535] [tap-frontapp] - fix extraction failures #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

romilqlik
Copy link

Description of change

Enhanced http.py to handle rate-limiting using back off strategy.
Added support for X-Ratelimit-Remaining and X-Ratelimit-Reset headers.

Manual QA steps

Ran unit tests:
test_client.py
Circle CI passes.

Risks

Any dependency might affect singer execution.

Rollback steps

  • revert this branch

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

@RushiT0122 RushiT0122 changed the base branch from master to feature/SAC-27536-frontapp-libraries May 20, 2025 04:42
@romilqlik romilqlik force-pushed the feature/SAC-27536-frontapp-libraries branch from 976e331 to 9f6165a Compare May 20, 2025 10:39
@romilqlik romilqlik force-pushed the feature/SAC-27535-frontapp branch from 5b4cdab to 065489b Compare May 21, 2025 12:49
Comment on lines -18 to -26
#def check_authorization(atx):
# atx.client.get('/settings')


# Some taps do discovery dynamically where the catalog is read in from a
# call to the api but with the odd frontapp structure, we won't do that
# here we never use atx in here since the schema is from file but we
# would use it if we pulled schema from the API def discover(atx):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is removed?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed because this block was originally part of the older inlines discover() in init.py.
Since the discovery logic has now been moved to discover.py, the comments has been cleaned up.

import os
import sys
import json

import singer
from singer import utils
from singer.catalog import Catalog, CatalogEntry, Schema
from singer.catalog import Catalog
from . import streams
Copy link
Contributor

@rdeshmukh15 rdeshmukh15 May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import from . import streams is not getting used anywhere in the file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it

@@ -0,0 +1,38 @@
"""Module for syncing selected FrontApp streams using Singer framework."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need of file level docstrings. Can you please remove wherever added.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it

romilqlik added 10 commits May 22, 2025 16:27
Removed type annotations from the file as per review
Removed type annotations from file as per review
Removed type annotations as per review
Removed type annotations as per comment
Removed unnecessary header as per review
Removed header comment
Removed file level docstring
Copy link
Author

@romilqlik romilqlik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly review

Comment on lines -18 to -26
#def check_authorization(atx):
# atx.client.get('/settings')


# Some taps do discovery dynamically where the catalog is read in from a
# call to the api but with the odd frontapp structure, we won't do that
# here we never use atx in here since the schema is from file but we
# would use it if we pulled schema from the API def discover(atx):

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed because this block was originally part of the older inlines discover() in init.py.
Since the discovery logic has now been moved to discover.py, the comments has been cleaned up.

@rdeshmukh15 rdeshmukh15 merged commit 62f087f into feature/SAC-27536-frontapp-libraries May 29, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants