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

Support for registering custom converters #93

Closed
1 task done
lovetoburnswhen opened this issue Mar 16, 2022 · 6 comments · Fixed by #129
Closed
1 task done

Support for registering custom converters #93

lovetoburnswhen opened this issue Mar 16, 2022 · 6 comments · Fixed by #129
Labels
enhancement New feature or request

Comments

@lovetoburnswhen
Copy link
Contributor

lovetoburnswhen commented Mar 16, 2022

Checklist

  • There are no similar issues or pull requests for this yet.

Is your feature related to a problem? Please describe.

There doesn't seem to be an obvious way to register converter functions with @converts or subclass ModelConverter.

This might also be a bug where ModelConverterBase.get_converter is unable to recognize TypeDecorator types that extend a type that already has a converter.

Describe the solution you would like.

Possibly utilizing a global registry for @converts.

Describe alternatives you considered

No response

Additional context

Encountered while trying to create a ModelAdmin for a SQLModel (related to #57)

Exception: Could not find field converter for column name (<class 'sqlmodel.sql.sqltypes.AutoString'>). where AutoString extends String

EDIT:
Got it to work by setting the sa_column= on the SQLModel field:

class MyModel(SQLModel):
    # name: str = Field(..., index=True) # broken
    name: str = Field(..., sa_column=Column(String(length=512))) # works

I believe the feature request still has value

@aminalaee
Copy link
Owner

Hi,
To be honest, I think we should first start integrating SQLModel first, and see how it goes.
After that we can decide to allow custom converters and how we can approach that.

Thanks for the suggestion.

@aminalaee aminalaee added the enhancement New feature or request label Mar 17, 2022
@aminalaee
Copy link
Owner

I've created a PR for SQLModel support, but I think this will solve the issue here too.
#94

Basically this will enable classes inheriting from TypeDecorator and handle them properly.

@lovetoburnswhen
Copy link
Contributor Author

Nice! However the PR does break sqlalchemy_utils.types.ChoiceType

  File "/Users/app/.venv/lib/python3.9/site-packages/sqladmin/models.py", line 635, in scaffold_form
    return await get_model_form(
                 └ <function get_model_form at 0x104e04a60>
  File "/Users/app/.venv/lib/python3.9/site-packages/sqladmin/forms.py", line 297, in get_model_form
    field = await converter.convert(
                  │         └ <function ModelConverterBase.convert at 0x104e1b550>
                  └ <sqladmin.forms.ModelConverter object at 0x1078f2fa0>
  File "/Users/app/.venv/lib/python3.9/site-packages/sqladmin/forms.py", line 105, in convert
    converter = self.get_converter(prop)
                │    │             └ <ColumnProperty at 0x105a27240; schedule_type>
                │    └ <function ModelConverterBase.get_converter at 0x104e1b4c0>
                └ <sqladmin.forms.ModelConverter object at 0x1078f2fa0>
  File "/Users/app/.venv/lib/python3.9/site-packages/sqladmin/forms.py", line 75, in get_converter
    if col_type.impl.__name__ in self.converters:  # type: ignore
       │        │                │    └ {'Boolean': <bound method ModelConverter.conv_Boolean of <sqladmin.forms.ModelConverter object at 0x1078f2fa0>>, 'dialects.ms...
       │        │                └ <sqladmin.forms.ModelConverter object at 0x1078f2fa0>
       │        └ Unicode(length=255)
       └ <class 'sqlalchemy_utils.types.choice.ChoiceType'>

AttributeError: 'Unicode' object has no attribute '__name__'

One benefit of being able to register custom converters is so that you can bypass the usual ModelConverter flow. Passing in form_overrides doesn't work either since it tries to get the converter before it looks at override

@aminalaee aminalaee linked a pull request Apr 7, 2022 that will close this issue
@okapies
Copy link
Contributor

okapies commented Jun 15, 2022

It seems this issue has not been solved yet, right? I need to add my converter to support the custom column types. @aminalaee

ref. #176.

@aminalaee
Copy link
Owner

@okapies After the PR was merged you should be able to override this behaviour by setting form_overrides.
But I see you've proposed a solution which might be simpler, I will take a look next week. Thank you for the contribution.

@okapies
Copy link
Contributor

okapies commented Jun 15, 2022

Thanks, I'll try it. IMHO, I think it may be useful if ModelAdmin accepts converters setting (type -> ConverterCallable or column -> ConverterCallable?) that customizes ModelConverter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants