feat(sheets): add include_formulas parameter to read_sheet_values#633
Merged
taylorwilsdon merged 6 commits intotaylorwilsdon:mainfrom Apr 1, 2026
Merged
Conversation
Adds an opt-in include_formulas: bool = False parameter to read_sheet_values. When True, a second values().get() call is made with valueRenderOption="FORMULA" and the result is appended as a formatted section listing any cells whose value is a formula string (i.e. starts with "="). Plain value cells are silently skipped. The parameter defaults to False so there is no performance impact on existing callers. Closes taylorwilsdon#632
Contributor
📝 WalkthroughWalkthroughAdds optional formula extraction to sheet reads: new helper fetches raw formulas via Sheets API Changes
Sequence DiagramsequenceDiagram
participant Client
participant read_sheet_values
participant _fetch_cell_formulas
participant GoogleSheetsAPI as Google Sheets API
participant _format_sheet_formula_section
Client->>read_sheet_values: read_sheet_values(..., include_formulas=True)
rect rgba(200, 200, 200, 0.5)
read_sheet_values->>GoogleSheetsAPI: values().get(valueRenderOption="FORMATTED_VALUE")
GoogleSheetsAPI-->>read_sheet_values: formatted_values
end
alt include_formulas == True
rect rgba(100, 150, 200, 0.5)
read_sheet_values->>_fetch_cell_formulas: _fetch_cell_formulas(service, spreadsheet_id, resolved_range, formatted_values)
_fetch_cell_formulas->>GoogleSheetsAPI: values().get(valueRenderOption="FORMULA") via asyncio.to_thread
GoogleSheetsAPI-->>_fetch_cell_formulas: formula_values
_fetch_cell_formulas->>_format_sheet_formula_section: format collected {cell, formula} pairs (truncate)
_format_sheet_formula_section-->>_fetch_cell_formulas: formatted_formula_block
_fetch_cell_formulas-->>read_sheet_values: formula_section, formula_values
end
end
read_sheet_values-->>Client: combined output (includes formula_section when requested)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gsheets/sheets_helpers.py`:
- Around line 989-995: The current except block in the read_sheet_values path
(when include_formulas=True) logs and returns an empty string, making errors
indistinguishable from legitimate empty formula results; instead, in the except
for fetching formula values (the block that logs "[read_sheet_values] Failed
fetching formula values..."), do not return "", but detect Google API
rate-limit/quota errors and raise MCP ToolExecutionError with the original
exception attached (or re-raise the original exception for non-rate-limit cases)
so callers can distinguish failures from empty results; update the exception
handling in the function/method that calls the formula fetch (e.g.,
read_sheet_values / the formula fetch helper) to raise ToolExecutionError for
quota/rate-limit errors and avoid converting all exceptions into an empty
string.
- Around line 999-1005: The reported cell addresses are computed from A1 because
row_idx/col_idx are not offset by the resolved_range start; parse the starting
column and row from resolved_range (e.g., extract start_col_label and
start_row_number), convert start_col_label to a zero-based start_col_index, then
when building each cell entry in the loop use adjusted indices (actual_col_index
= start_col_index + col_idx and actual_row_number = start_row_number + row_idx -
1) and call _index_to_column(actual_col_index) to form the correct cell label
before appending to formulas; keep references to formula_values, row_idx,
col_idx, resolved_range, _index_to_column, and _format_sheet_formula_section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d18c0ce-5be1-49a2-aa8c-86dea03691e2
📒 Files selected for processing (2)
gsheets/sheets_helpers.pygsheets/sheets_tools.py
- Raise exceptions instead of silently returning "" on API failure, so quota/auth/network errors are distinguishable from empty results - Offset row/col indices by the resolved_range start position so formulas in ranges like C5:E10 report correct cell addresses (e.g. C5) instead of always starting from A1
…into feat/include-formulas-read-sheet-values
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
gsheets/sheets_helpers.py (1)
975-995:⚠️ Potential issue | 🟠 MajorDo not swallow formula-fetch failures in
include_formulaspath.Line 988–995 converts API failures into “no formulas,” which is ambiguous and can lead to unsafe write-back behavior.
Proposed fix
- Returns an empty section and empty values list if the request fails. + Raises on API failures so callers can distinguish errors from empty formula results. """ - try: - result = await asyncio.to_thread( - service.spreadsheets() - .values() - .get( - spreadsheetId=spreadsheet_id, - range=resolved_range, - valueRenderOption="FORMULA", - ) - .execute - ) - except Exception as exc: - logger.warning( - "[read_sheet_values] Failed fetching formula values for range '%s': %s", - resolved_range, - exc, - ) - return "", [] + result = await asyncio.to_thread( + service.spreadsheets() + .values() + .get( + spreadsheetId=spreadsheet_id, + range=resolved_range, + valueRenderOption="FORMULA", + ) + .execute + )As per coding guidelines "Catch rate-limit and quota errors from Google APIs and surface them as MCP ToolExecutionError."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gsheets/sheets_helpers.py` around lines 975 - 995, The current read_sheet_values implementation swallows API exceptions and returns an empty section/values which masks failures in the include_formulas path; instead, catch the Google API errors and surface them as an MCP ToolExecutionError (or re-raise) so callers can handle rate-limit/quota problems—update the except block in read_sheet_values to detect Google API errors (e.g., HttpError with 429/403 quota messages or underlying exceptions) and raise ToolExecutionError with the original exception/details (while still logging), rather than returning "", [] so include_formulas and other callers don't treat failures as "no formulas."tests/gsheets/test_read_sheet_values.py (1)
47-57:⚠️ Potential issue | 🟠 MajorThis test asserts the wrong failure behavior for formula fetch errors.
Line 53 currently expects a successful read when formula fetch fails, but formula-fetch failures should propagate (not be treated as “no formula cells”).
Proposed fix
`@pytest.mark.asyncio` async def test_read_sheet_values_tolerates_formula_fetch_failures(): service = _create_mock_service( {"range": "Sheet1!A1:A1", "values": [["1"]]}, RuntimeError("formula fetch failed"), ) - - result = await _call_read_sheet_values(service, include_formulas=True) - - assert "Successfully read 1 rows" in result - assert "Row 1: ['1']" in result - assert "Formula cells in range" not in result + with pytest.raises(RuntimeError, match="formula fetch failed"): + await _call_read_sheet_values(service, include_formulas=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gsheets/test_read_sheet_values.py` around lines 47 - 57, The test test_read_sheet_values_tolerates_formula_fetch_failures is asserting success when the formula fetch throws RuntimeError, but formula-fetch failures should propagate; update the test to expect the RuntimeError (or the exact error message) to be raised by calling _call_read_sheet_values(..., include_formulas=True) instead of asserting successful output, e.g., use an assertRaises/pytest.raises around the call to _call_read_sheet_values to verify the RuntimeError from the mocked service is propagated.
🧹 Nitpick comments (1)
tests/gsheets/test_read_sheet_values.py (1)
10-16: Usehttpretty+ canned fixtures for Google API mocking in tests.This helper currently builds a chained
Mock; the repository test guideline asks for HTTP-level mocks with canned fixtures.As per coding guidelines "Mock Google APIs with httpretty and canned fixtures; do not hit live services in CI."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gsheets/test_read_sheet_values.py` around lines 10 - 16, The test helper _create_mock_service builds a chained Mock for Sheets API calls; replace this with HTTP-level mocks using httpretty and canned JSON fixtures to comply with repo guidelines. Update tests that call _create_mock_service to instead register httpretty responses for the Google Sheets values.get endpoint (matching the expected URL and query params) and return the canned fixture payloads (stored under tests/fixtures or similar); ensure httpretty is enabled/disabled in test setup/teardown so no real network calls occur. Use the same sequential responses by registering multiple httpretty responses in order to simulate side_effect behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@gsheets/sheets_helpers.py`:
- Around line 975-995: The current read_sheet_values implementation swallows API
exceptions and returns an empty section/values which masks failures in the
include_formulas path; instead, catch the Google API errors and surface them as
an MCP ToolExecutionError (or re-raise) so callers can handle rate-limit/quota
problems—update the except block in read_sheet_values to detect Google API
errors (e.g., HttpError with 429/403 quota messages or underlying exceptions)
and raise ToolExecutionError with the original exception/details (while still
logging), rather than returning "", [] so include_formulas and other callers
don't treat failures as "no formulas."
In `@tests/gsheets/test_read_sheet_values.py`:
- Around line 47-57: The test
test_read_sheet_values_tolerates_formula_fetch_failures is asserting success
when the formula fetch throws RuntimeError, but formula-fetch failures should
propagate; update the test to expect the RuntimeError (or the exact error
message) to be raised by calling _call_read_sheet_values(...,
include_formulas=True) instead of asserting successful output, e.g., use an
assertRaises/pytest.raises around the call to _call_read_sheet_values to verify
the RuntimeError from the mocked service is propagated.
---
Nitpick comments:
In `@tests/gsheets/test_read_sheet_values.py`:
- Around line 10-16: The test helper _create_mock_service builds a chained Mock
for Sheets API calls; replace this with HTTP-level mocks using httpretty and
canned JSON fixtures to comply with repo guidelines. Update tests that call
_create_mock_service to instead register httpretty responses for the Google
Sheets values.get endpoint (matching the expected URL and query params) and
return the canned fixture payloads (stored under tests/fixtures or similar);
ensure httpretty is enabled/disabled in test setup/teardown so no real network
calls occur. Use the same sequential responses by registering multiple httpretty
responses in order to simulate side_effect behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf07c019-4033-405e-a3d8-0cb9f51b5cd5
📒 Files selected for processing (3)
gsheets/sheets_helpers.pygsheets/sheets_tools.pytests/gsheets/test_read_sheet_values.py
🚧 Files skipped from review as they are similar to previous changes (1)
- gsheets/sheets_tools.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @taylorwilsdon, long-time user here. I ran into a problem where I used
read_sheet_valuesto inspect a range before writing back to it, and silently blew away a bunch of cross-tab formula references because the tool returns computed values rather than formula strings. Opened #632 with more detail.This PR adds
include_formulas: bool = Falsetoread_sheet_values. When enabled, it makes a secondvalues().get()call withvalueRenderOption="FORMULA"and appends a section to the output listing any cells whose value is a formula. Defaults to False so there is no change in behaviour for existing callers.The implementation follows the same opt-in bool-flag pattern as
include_notes(added in #580), just using the simplervalues().get()path rather thanspreadsheets.get()withincludeGridData=True(formulas don't need grid data).Changes
gsheets/sheets_helpers.py: adds_fetch_cell_formulas()and_format_sheet_formula_section()gsheets/sheets_tools.py: addsinclude_formulasparam, imports and calls the new helper, appends result to return stringType of Change
Testing
C5:E10) — confirmed cell addresses in output are now correctly offset rather than always starting from A1Resolves #632.
Happy to adjust anything if the approach doesn't fit how you want the tool to evolve.
Summary by CodeRabbit
New Features
Tests