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

Enable experimental_python_types by default and change the name #305

Closed
1 task done
damian3031 opened this issue Dec 22, 2022 · 8 comments · Fixed by #308
Closed
1 task done

Enable experimental_python_types by default and change the name #305

damian3031 opened this issue Dec 22, 2022 · 8 comments · Fixed by #308
Assignees

Comments

@damian3031
Copy link
Member

Describe the feature

Enable experimental_python_types by default.
Change this parameter name to legacy_python_types.

Describe alternatives you've considered

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@damian3031 damian3031 self-assigned this Dec 22, 2022
@hashhar
Copy link
Member

hashhar commented Dec 22, 2022

legacy_python_types might not be descriptive enough. What about raw_results/untyped_results?

@hashhar
Copy link
Member

hashhar commented Dec 22, 2022

Also as a part of this we should make sure we document clearly the limitations of the new default and mention how to workaround them when needed.

@damian3031
Copy link
Member Author

damian3031 commented Dec 23, 2022

@hashhar If I understand correctly, logic behind utilising current passed parameter experimental_python_types is here

So if this parameter is True, then type mapping is performed.

So, after changing this parameter name to raw_results/untyped_results, setting raw_results to True would cause to perform mapping, shouldn't it be the opposite?

Maybe we could name this parameter differently, instead of raw_results/untyped_results maybe sth like typed_results/mapped_results? and then by default mapped_results == True and mapping is performed?

Or maybe I could add negation here, rename to raw_result and leave default value to False, so by default raw_result == False and mapping is performed?

@hashhar
Copy link
Member

hashhar commented Dec 23, 2022

setting raw_results to True would cause to perform mapping, shouldn't it be the opposite?

Yes. The name change also means the logic needs to be inverted.

Or maybe I could add negation here, rename to raw_result and leave default value to False, so by default raw_result == False and mapping is performed?

This seems good to me.

@hovaesco / @mdesmet any opinion?

@hovaesco
Copy link
Member

Sounds good to me, I'm not in favour of raw_results name, untyped_results is better or maybe legacy_untyped_results?

@mdesmet
Copy link
Contributor

mdesmet commented Dec 23, 2022

untyped seems a little weird as the results are always typed. Maybe legacy_primitive_types?

@hashhar
Copy link
Member

hashhar commented Dec 23, 2022

are the results typed or are they always string?

@mdesmet
Copy link
Contributor

mdesmet commented Dec 23, 2022

The api only returns string or int. Other types are explicitly mapped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants