[v0.17.0] - Check for record type will likely fail for dict records

[joelghill]

Haven't actually tested, but the logic in is_record_type in atproto.xrpc_client.models.utils will likely fail for dicts:

def is_record_type(model: t.Union[ModelBase, dict], types_module) -> bool:
    if isinstance(model, RecordModelBase) and hasattr(types_module, 'Main'):
        # for now records in Main. could be broken late
        if isinstance(model, dict):  # custom record
            return types_module.Main._type == model.get('$type')

        return types_module.Main._type == model._type

    return False

I think it should be:

def is_record_type(model: t.Union[ModelBase, dict], types_module) -> bool:
    if isinstance(model, RecordModelBase) and hasattr(types_module, 'Main'):
        # for now records in Main. could be broken late
        return types_module.Main._type == model._type

    if isinstance(model, dict):  # custom record
        return types_module.Main._type == model.get('$type')

    return False

[MarshalX]

wow. looks of course legit due to invalid isinstance() logic except how it should work

i think it should be:

def is_record_type(model: t.Union[ModelBase, dict], types_module) -> bool:
    # for now records in Main. could be broken late
    if hasattr(types_module, 'Main'):
        if isinstance(model, RecordModelBase):
            return types_module.Main._type == model._type
        if isinstance(model, dict):  # custom record
            return types_module.Main._type == model.get('$type')

    return False

we don't rly support unknown records from custom lexicons at all we case we don't know this custom lexicon... by "custom record" i mean an extended version of the base record (described by official lexicons). extended with additional fields, for example

[MarshalX]

more clear indents :

def is_record_type(model: t.Union[ModelBase, dict], types_module) -> bool:
    # for now, records in the Main class only. could be broken late
    if not hasattr(types_module, 'Main'):
        return False

    if isinstance(model, dict):  # custom (extended) record
        return types_module.Main._type == model.get('$type')

    return types_module.Main._type == model._type

[joelghill]

Thanks! I'll take another crack at this tomorrow if I have time.

Do you have guidelines on the formatting, listing, unit test, etc? I see you're using Black and Ruff?

[MarshalX]

Thanks! I'll take another crack at this tomorrow if I have time.

Do you have guidelines on the formatting, listing, unit test, etc? I see you're using Black and Ruff?

Not yet. All that you need is poetry run black . and poetry run ruff . --fix (of course after poetry install)

[MarshalX]

fixed in v0.0.18