Skip to content
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

Adding an oop example #71

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adding an oop example #71

wants to merge 7 commits into from

Conversation

orizwanft
Copy link
Collaborator

Using the National Park Services API to use an OOP based logic to build the connector. This also has a new format with a Readme to walk through the code and show the output in the database itself vs in the code comments.

Copy link

height bot commented Jan 8, 2025

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

for table in selected_table:
con = table(configuration=configuration)
schema_dict = con.assign_schema()
#print(f"Schema for table {con}: {schema_dict}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@@ -0,0 +1,3 @@
{
"api_key": ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"api_key": ""
"api_key": "<YOUR_API_KEY>"

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator

@fivetran-rishabhghosh fivetran-rishabhghosh left a comment

Choose a reason for hiding this comment

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

Please address above comments. Everything else looks good to me

Copy link
Collaborator

@varundhall varundhall left a comment

Choose a reason for hiding this comment

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

Please address the review comments

@@ -0,0 +1,3 @@
{
"api_key": ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

for table in selected_table:
con = table(configuration=configuration)
schema_dict = con.assign_schema()
#print(f"Schema for table {con}: {schema_dict}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This pull request introduces an OOP-based connector for the National Parks Service API, encapsulating logic for processing different tables (People, Parks, Articles, Alerts) and providing a new Readme to explain the connector’s structure and output.

  • Implements separate classes for each table inheriting from an NPS base class.
  • Refactors API fetching and data processing into modular components.
  • Adds a connector file to integrate and run the different table modules.

Reviewed Changes

File Description
people_table.py Adds the PEOPLE class with methods for data processing and schema definition.
nps_client.py Introduces the NPS client with API interaction and configuration handling.
park_table.py Implements the PARKS class for park data processing.
articles_table.py Details the ARTICLES class to map article data to a schema.
alerts_table.py Creates the ALERTS class for processing alert data.
Readme.md Provides documentation and usage instructions for the connector.
connector.py Integrates all table modules into a single connector for deployment.

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

examples/quickstart_examples/oop_example/people_table.py:75

  • Both the 'title' and 'description' fields are extracted using 'listingDescription'. Verify if this is intentional or if a distinct key should be used for one of them.
title = p.get("listingDescription", "No Title")  # Get title or default to "No Title"

examples/quickstart_examples/oop_example/people_table.py:4

  • [nitpick] Consider using CamelCase (e.g., People) for class names instead of all uppercase to align with common Python naming conventions.
class PEOPLE(NPS):

Copy link
Collaborator

@varundhall varundhall left a comment

Choose a reason for hiding this comment

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

Please test the changes once before merging.

@varundhall varundhall requested a review from 5tran-alexil March 4, 2025 11:30
@varundhall
Copy link
Collaborator

@5tran-alexil please review the readme.md file

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