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

Fix KeyError by adding check for _convert_dates #60539

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

Conversation

Uvi-12
Copy link
Contributor

@Uvi-12 Uvi-12 commented Dec 11, 2024

Closes #60536

This PR adds a check to prevent a KeyError when renaming columns by ensuring the original column name exists in the _convert_dates dictionary before updating it.

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Whenever changing behavior, please add tests.

@rhshadrach rhshadrach added Bug IO Stata read_stata, to_stata labels Dec 27, 2024
@Uvi-12
Copy link
Contributor Author

Uvi-12 commented Jan 2, 2025

Thanks for the PR! Whenever changing behavior, please add tests.

Thank you for the review. Please guide me on which test file would be the most appropriate for adding tests for this change.

@rhshadrach
Copy link
Member

pandas/tests/io/test_stata.py

@Uvi-12 Uvi-12 requested a review from rhshadrach January 3, 2025 08:15
@Uvi-12
Copy link
Contributor Author

Uvi-12 commented Jan 3, 2025

@rhshadrach I have added the tests, please check if they are correct.

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Comment on lines +2599 to +2606
writer._convert_dates = {"old_name": "converted_date"}
columns = ["new_name"]
original_columns = ["old_name"]
for c, o in zip(columns, original_columns):
if c != o and o in writer._convert_dates:
writer._convert_dates[c] = writer._convert_dates[o]
del writer._convert_dates[o]
assert writer._convert_dates == {"new_name": "converted_date"}
Copy link
Member

Choose a reason for hiding this comment

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

Can you instead test using the public API - provide a DataFrame with an name that will be changed, and a convert_dates, then call read_stata to check the result.

Comment on lines +2593 to +2594
def test_convert_dates_key_handling(tmp_path, version):
temp_file = tmp_path / "test.dta"
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the first line of test references the GitHub issue (or PR):

def test_convert_dates_key_handling(tmp_path, version):
    # GH#60536
    temp_file = tmp_path / "test.dta"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IO Stata read_stata, to_stata
Projects
None yet
2 participants