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 logic bug - add_offset is in encoding, not attrs. #6851

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

mankoff
Copy link
Contributor

@mankoff mankoff commented Jul 29, 2022

_pop_to does a pop operation - it removes the key/value pair. So the line above this change will remove add_offset from attrs if it exists. The second line then checks for add_offset in attrs which should always be False.

This is implemented in #6812 but the work there is significantly more involved (see for example different PR addressing the same problem, #2304), so I'm pulling this fix out and submitting it as its own PR.

@dcherian
Copy link
Contributor

dcherian commented Aug 1, 2022

Thanks @mankoff Is there a test we could add?

@mankoff
Copy link
Contributor Author

mankoff commented Aug 1, 2022

Thanks @mankoff Is there a test we could add?

There's a whole table of tests! #2304 (comment)

But now I'm building a test for the code as-is, which isn't CF-compliant. Is this worth writing?

@dcherian
Copy link
Contributor

dcherian commented Aug 1, 2022

ah let's just add that to the other PR.

Thanks! This is a great contribution

@dcherian dcherian merged commit 3c98ec7 into pydata:main Aug 1, 2022
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.

2 participants