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

Single word title for Google Spreadsheet #6510

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

Conversation

immortalcodes
Copy link

@immortalcodes immortalcodes commented Oct 9, 2023

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

This fixes and works on the issue #6409
The query_runner for google spreadsheet has now some refactoring and recognise single word title without quotes

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@immortalcodes immortalcodes changed the title Update google_spreadsheets.py Single word title for Google Spreadsheet Oct 9, 2023
Copy link
Contributor

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

@immortalcodes thanks for the PR! I left a few comments - the refactor of this function isn't as straightforward as it looks. We should strive to simplify this function and make it more understandable.

redash/query_runner/google_spreadsheets.py Outdated Show resolved Hide resolved
redash/query_runner/google_spreadsheets.py Outdated Show resolved Hide resolved
redash/query_runner/google_spreadsheets.py Outdated Show resolved Hide resolved
@justinclift
Copy link
Member

Looks like this PR has a formatting problem that needs fixing:

Run ruff check .
redash/query_runner/google_spreadsheets.py:122:1: W293 [*] Blank line contains whitespace

@immortalcodes
Copy link
Author

I have used a delimiter to have string and title both as a string and still be able to differentiate between them

@immortalcodes
Copy link
Author

I guess this might require changing the unit tests as well

@@ -100,18 +100,20 @@ def __init__(self, worksheet_title):
def parse_query(query):
values = query.split("|")
key = values[0] # key of the spreadsheet
worksheet_num_or_title = 0 # A default value for when a number of inputs is invalid
worksheet_pointer = ' 1' # A default value for when a number of inputs is invalid
Copy link
Contributor

Choose a reason for hiding this comment

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

@immortalcodes hey, thanks for updating this PR. I think I might have misspoken though, or I wasn't clear in my communication - we want to separate parse_query into two versions of a similar function, where one uses a worksheet number and the other uses the title, and we want to call the correct function depending on what the user inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we don't have to worry about things like a space prefix on a string signifying anything; we should keep the data as separate as possible from the code logic.

@guidopetri
Copy link
Contributor

@immortalcodes hey, just checking in. Any interest in finishing this PR still?

@immortalcodes
Copy link
Author

@immortalcodes hey, just checking in. Any interest in finishing this PR still?

Yup, was busy with some work

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