Skip to content

Fix RBI workbook preprocessing - #2172

Draft
rohitkumarbhagat wants to merge 3 commits into
datacommonsorg:masterfrom
rohitkumarbhagat:fix-rbi-workbook-preprocessing
Draft

Fix RBI workbook preprocessing#2172
rohitkumarbhagat wants to merge 3 commits into
datacommonsorg:masterfrom
rohitkumarbhagat:fix-rbi-workbook-preprocessing

Conversation

@rohitkumarbhagat

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the preprocess_files function in rbi_download.py to write processed Excel sheets to a temporary directory before replacing the original file, ensuring atomic writes and preventing file corruption on failure. It also introduces a unit test in rbi_download_test.py to verify this behavior. The review feedback suggests moving the safe_to_numeric helper function outside of the loop to avoid redefinition, replacing the deprecated applymap method with map for pandas compatibility, and avoiding asserting on exact log messages in the unit test to prevent brittleness.

Comment on lines +43 to +44
fatal.assert_called_once_with(
'Error processing source.xlsx: transform failed')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Avoid writing tests that assert on exact log messages, as they are brittle and prone to breaking when log formats or wording change. Instead, assert that the logging function was called.

            fatal.assert_called_once()
References
  1. Avoid writing tests that assert on log messages, as they are brittle.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.

1 participant