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

feat: support list in non-nested mutable json #51

Merged
merged 3 commits into from
Aug 20, 2023

Conversation

iloveitaly
Copy link
Contributor

No description provided.

@edelooff
Copy link
Owner

This makes very explicit what essentially was an unintended (but perhaps fine) behavior that's currently in the library: the top-level isn't always a dict.

That was (is?) the basic assumption, but the way NestedMutable is implemented (to support easy nesting), the top-level for that one was allowed to be a list rather than a dict, creating a weird difference between the nested mutable and 'flat mutable'.

The databases seem to be fine with JSON arrays as the top level, so I'm not sure this library should arbitrarily stick to just objects. And even RFC 4627 supports both arrays and objects at the top level, so this seems in line with that and should be supported.

@@ -50,14 +50,28 @@ def coerce(cls, key, value):
return NestedMutableList.coerce(key, value)
return super(cls).coerce(key, value)

class MutableListOrDict(Mutable):
Copy link
Owner

Choose a reason for hiding this comment

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

This should maintain 2 empty lines between definitions (and I guess I should really include black or flake8 with a plugin in the CI runs..)


def mutable_json_type(dbtype=JSON, nested=False):
"""Type creator for (optionally nested) mutable JSON column types.

The default backend data type is sqlalchemy.types.JSON, but can be set to
any other type by providing the `dbtype` parameter.
"""
mutable_type = NestedMutable if nested else MutableDict
mutable_type = NestedMutable if nested else MutableListOrDict
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not 100% sure on the name here, but that might be a bit of a wider concern, naming this NestedJson isn't possible as the name gets taken by the column type later (which I think is more generally useful in exports), but getting it to better capture its meaning and then align it with the Nested... variant would be nice.

Suggestions are welcome if you have any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I couldn't come up with a better name either :/

@iloveitaly
Copy link
Contributor Author

creating a weird difference between the nested mutable and 'flat mutable'.

yeah, this is what was unintuitive to me and the driver behind the change here.

Let me know if any other changes are needed!

@edelooff edelooff merged commit eca5838 into edelooff:master Aug 20, 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.

2 participants