Skip to content

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.

@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

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