Skip to content

Implement Symbol as Text in C-extension #327

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

Merged
merged 2 commits into from
Dec 28, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions amazon/ion/ioncmodule.c
Original file line number Diff line number Diff line change
@@ -1075,7 +1075,10 @@ static void ionc_add_to_container(PyObject* pyContainer, PyObject* element, BOOL
*/
iERR ionc_read_value(hREADER hreader, ION_TYPE t, PyObject* container, BOOL in_struct, uint8_t value_model) {
iENTER;
BOOL wrap_py_value = !value_model;

BOOL wrap_py_value = !(value_model & 1);
BOOL symbol_as_text = value_model & 2;

BOOL is_null;
ION_STRING field_name;
SIZE annotation_count;
@@ -1219,8 +1222,15 @@ iERR ionc_read_value(hREADER hreader, ION_TYPE t, PyObject* container, BOOL in_s
{
ION_STRING string_value;
IONCHECK(ion_reader_read_string(hreader, &string_value));
py_value = ion_string_to_py_symboltoken(&string_value);
ion_nature_constructor = _ionpysymbol_fromvalue;
if (!symbol_as_text) {
py_value = ion_string_to_py_symboltoken(&string_value);
ion_nature_constructor = _ionpysymbol_fromvalue;
} else if (ion_string_is_null(&string_value)) {
_FAILWITHMSG(IERR_INVALID_STATE, "Cannot emit symbol with undefined text when SYMBOL_AS_TEXT is set.");
} else {
py_value = ion_build_py_string(&string_value);
ion_nature_constructor = _ionpytext_fromvalue;
}
break;
}
case tid_STRING_INT:
@@ -1475,8 +1485,8 @@ PyObject* ionc_read(PyObject* self, PyObject *args, PyObject *kwds) {
&value_model, &text_buffer_size_limit)) {
FAILWITH(IERR_INVALID_ARG);
}
if (value_model > 1) {
FAILWITHMSG(IERR_INVALID_ARG, "Only ION_PY and MIXED value models are currently supported!")
if (value_model > 3) {
_FAILWITHMSG(IERR_INVALID_ARG, "Only ION_PY, MAY_BE_BARE and SYMBOL_AS_TEXT value models are currently supported!")
}

iterator = PyObject_New(ionc_read_Iterator, &ionc_read_IteratorType);
49 changes: 39 additions & 10 deletions tests/test_simpleion.py
Original file line number Diff line number Diff line change
@@ -741,22 +741,51 @@ def test_bare_values(params):
assert ion_equals(value, expectation)


def test_value_models_flags():
@parametrize(
("foo", IonPyValueModel.ION_PY, IonPySymbol),
("foo", IonPyValueModel.MAY_BE_BARE, SymbolToken),
("foo", IonPyValueModel.SYMBOL_AS_TEXT, IonPyText, IonType.SYMBOL),
("foo", IonPyValueModel.MAY_BE_BARE | IonPyValueModel.SYMBOL_AS_TEXT, str),
("foo::bar", IonPyValueModel.MAY_BE_BARE, IonPySymbol),
("foo::bar", IonPyValueModel.MAY_BE_BARE | IonPyValueModel.SYMBOL_AS_TEXT, IonPyText, IonType.SYMBOL),
)
def test_value_model_flags(params):
# This function only tests c extension
if not c_ext:
return

ion_text = "31"
value = simpleion.load_extension(StringIO(ion_text), value_model=IonPyValueModel.ION_PY)
assert type(value) is IonPyInt
value = simpleion.load_extension(StringIO(ion_text), value_model=IonPyValueModel.MAY_BE_BARE)
assert type(value) is int
if len(params) == 3:
ion_text, value_model, expected_type = params
expected_ion_type = None
else:
ion_text, value_model, expected_type, expected_ion_type = params

try:
value = simpleion.load_extension(StringIO(ion_text), value_model=value_model)
assert type(value) is expected_type
if expected_ion_type:
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.

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

ion_text = """
$ion_symbol_table::{ symbols:[ null ] }
$10
"""
with raises(IonException, match="Cannot emit symbol with undefined text"):
simpleion.load_extension(StringIO(ion_text), value_model=IonPyValueModel.SYMBOL_AS_TEXT)
assert False
except:
pass


def test_invalid_value_model_flag():
# This function only tests c extension
if not c_ext:
return

with raises(IonException, match="value models are currently supported"):
simpleion.load_extension(StringIO("foo"), value_model=IonPyValueModel.STRUCT_AS_STD_DICT)


# See issue https://github.com/amazon-ion/ion-python/issues/232