Skip to content

Feature/sac 27535 frontapp #29

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

romilqlik
Copy link

Description of change

Refactored the Tap FrontApp connector to modularize discovery and sync logic:
Moved discover() to a dedicated discover.py file for clarity and reuse.
Introduced sync.py to handle sync operations with better structure and logging.
Cleaned up and simplified init.py to delegate responsibilities to modular files.

Manual QA steps

Ran the discover mode

Risks

As of now NA.

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

refs = {}
for sub_stream_id in dependencies:
refs[sub_stream_id] = load_schema(sub_stream_id)
refs = {sub_id: load_schema(sub_id) for sub_id in dependencies}
Copy link

@RushiT0122 RushiT0122 May 22, 2025

Choose a reason for hiding this comment

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

Not a blocking change

Suggested change
refs = {sub_id: load_schema(sub_id) for sub_id in dependencies}
refs = {sub_stream_id: load_schema(sub_stream_id) for sub_stream_id in dependencies}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it

LOGGER.info("Selected streams: %s", streams_to_sync)

last_stream = singer.get_currently_syncing(state)
LOGGER.info("Last stream synced (if resuming): %s", last_stream)
Copy link

@RushiT0122 RushiT0122 May 22, 2025

Choose a reason for hiding this comment

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

Suggested change
LOGGER.info("Last stream synced (if resuming): %s", last_stream)
LOGGER.info("Currently syncing: %s", last_stream)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it

LOGGER = singer.get_logger()


def discover() -> Catalog:

Choose a reason for hiding this comment

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

To keep consistency with existing implementation, please remove type annotation from the new implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it

romilqlik added 6 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
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 all the annotation changes.

romilqlik added 4 commits May 22, 2025 17:03
Removed unnecessary header as per review
Removed header comment
Removed file level docstring
@@ -25,4 +25,4 @@
"schemas": ["tap_frontapp/schemas/*.json"]
},
include_package_data=True
)
)

Choose a reason for hiding this comment

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

EOF



if __name__ == "__main__":
unittest.main()

Choose a reason for hiding this comment

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

Add EOF

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.

Added EOF

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