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

Implement Symbol as Text in C-extension #327

Merged
merged 2 commits into from
Dec 28, 2023
Merged

Conversation

rmarrowstone
Copy link
Contributor

@rmarrowstone rmarrowstone commented Dec 28, 2023

By default the IonPy value for a Symbol is IonPySymbol and the bare
value is a SymbolToken. With the SYMBOL_AS_TEXT flag set, the IonPy
value for a Symbol is IonPyText, and the bare value is str.

This makes Symbol handling simpler if one only cares about the text
value. It also improves load performance for symbols significantly.

Symbols with undefined text cannot be emitted with this flag set and
will raise exceptions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

By default the IonPy value for a Symbol is IonPySymbol and the bare
value is a SymbolToken. With the SYMBOL_AS_TEXT flag set, the IonPy
value for a Symbol is IonPyText, and the bare value is str.

This makes Symbol handling simpler if one only cares about the text
value. It also improves load performance for symbols significantly.

Symbols with undefined text cannot be emitted with this flag set and
will raise exceptions.
@rmarrowstone rmarrowstone marked this pull request as ready for review December 28, 2023 21:49
@rmarrowstone rmarrowstone requested a review from tgregg December 28, 2023 21:50
assert value.ion_type == expected_ion_type


def test_undefined_symbol_text_as_text():
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this test might need

    # This function only tests c extension
    if not c_ext:
        return

So that it doesn't fail on Pypy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof yea.

@rmarrowstone rmarrowstone merged commit 316b14d into master Dec 28, 2023
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.

None yet

2 participants