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

Data 1762 refactor gsheets to os codebase #48

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

Conversation

welmaris
Copy link
Collaborator

@welmaris welmaris commented Sep 5, 2024

Issue number

Data 1762

Importing gsheet document with suggested edits for the changes with the new script.

Gsheets in OS

  • it can still work with all the possible args. None are required.
  • There are load tables in a project starting with DATA-1762 that can be used to see my tests / redo the tests (I added some descriptions for some in the properties regarding its test)

@welmaris welmaris requested a review from Shrikey September 5, 2024 14:41
Copy link
Contributor

@Shrikey Shrikey left a comment

Choose a reason for hiding this comment

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

Looks good! Some small improvements

wherescape/connectors/gsheet/readme.md Outdated Show resolved Hide resolved
self.content = None
self.column_types = None

def set_spreadsheet(self, url: str = "", name: str = ""):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make this into a separate function instead of the init

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added this and all setters to the innit. Included a test param, so I could test them separately. It has to be set purposefully, so it shouldn't cause unintended issues

try:
row = self.worksheet.get(header_range)[0]
self.header = ["column_" + str(i + 1) if value == "" else value for i, value in enumerate(row)]
self.content.pop(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldnt be done in the case of a header range, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is because the header is still considered included in content. There is the issue that it assumes the first row of the content is the header, which could probably be improved upon, but I don't have an immediate idea of a solution. Might be a decent small ticket for improvement for later

wherescape/connectors/gsheet/load_data.py Show resolved Hide resolved
wherescape/connectors/gsheet/load_data.py Outdated Show resolved Hide resolved
Gsheet Connector for WhereSCape. Takes care of creating metadata for a loading data and uploading the data from a gsheet file.

# Preparation
An authentication user is required from Wherescape. For this, a client secret has to be created in the Google API Console.
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be saved in the wherescape connection details

Create a new python host script and add it to the load table. Example code:

```
from wherescape_os.wherescape.connectors.gsheet.create_metadata import gsheet_create_metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

update to conventions

@welmaris welmaris self-assigned this Oct 18, 2024
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.

2 participants