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

Convert None to "" in convert_columns_to_str #728

Merged
merged 1 commit into from
Aug 24, 2022
Merged

Conversation

Jason94
Copy link
Collaborator

@Jason94 Jason94 commented Aug 12, 2022

The function parsons.Table.convert_columns_to_str() converts None to "None". I think that converting it to the empty string is more intuitive and more helpful. It does make it harder to deserialize the result of convert_columns_to_str, but the str function is not generally deserializable anyway.

@shaunagm
Copy link
Collaborator

This seems straightforward. I wish there was a way to check with users, though! Can we think of any case or workflow in which someone might be relying on getting "None" vs an empty string?

@Jason94
Copy link
Collaborator Author

Jason94 commented Aug 16, 2022

In theory I could imagine a case where someone does something like this:

strs = table.convert_columns_to_str()
for row in strs.to_dict():
    if row['col'] == "None":
        do_something()

But I think that is somewhat unlikely. The purpose of this function seems to be for presentation, not for logic. And there is no reason you couldn't check for None values before calling convert_columns_to_str(). So you could say that relying on this behavior doesn't allow you to do anything you couldn't do before; just do it in a more confusing fashion. (In fact the above code has the problem that if the original table had "None" values in 'col' they would call the function, which is probably not what you would want.)

You're right to be cautious because it is an API breaking change. But the existing behavior is IMO niche and confusing enough that I think it's warranted. Certainly worth noting on release though, at the very least.

@shaunagm shaunagm added the breaking change applied only to PRs, indicates when something has a breaking change to be flagged in release notes label Aug 22, 2022
@shaunagm
Copy link
Collaborator

shaunagm commented Aug 22, 2022

I've created a 'breaking change' label to help us flag these kinds of things in releases. With that handled, this looks good.

Do you have merge powers? I forget - if not I can merge it for you.

@Jason94 Jason94 merged commit 1aae2dd into move-coop:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change applied only to PRs, indicates when something has a breaking change to be flagged in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants