-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
self.content = None | ||
self.column_types = None | ||
|
||
def set_spreadsheet(self, url: str = "", name: str = ""): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update to conventions
…also not fully updated yet
Issue number
Data 1762
Importing gsheet document with suggested edits for the changes with the new script.
Gsheets in OS