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

Add TypedDict for params and data arguments; add type hint for kwargs #166

Merged
merged 9 commits into from
Sep 28, 2023

Conversation

DXsmiley
Copy link
Contributor

Here's the changes that I actually wanted to make before getting caught up with the non-determinism lol

I was getting a lot of errors from my typechecker (pyright) while trying to use parts of the library (it doesn't like function types annotated with a raw dict), so I've replaced these with TypedDict types to more accurately reflect the values that the functions actually allow. They're approximately the same as the equivalent model types.

I've also annotated **kwargs on these functions with Any since the typechecker was complaining about it being unannotated, even when I wasn't supplying a value for it. It might be possible to more accurately type it with Unpack, however I think that's going to take enough effort to warrant a separate PR.

This change should only impact static type checkers and shouldn't impact runtime behaviour.

Copy link
Owner

@MarshalX MarshalX left a comment

Choose a reason for hiding this comment

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

general quesiton: will it give the ability to pass arbitrary keys to such dicts without problems (i mean without typing errors)? for example user may want to create records with custom fields that are not described in lexicon but could be saved in repo

@MarshalX
Copy link
Owner

general question 2: pydantic stores all definitions (including resolved types) of fields inside each model. Maybe is it possible to reuse it for dicts somehow? just and idea

@MarshalX
Copy link
Owner

general question 2: pydantic stores all definitions (including resolved types) of fields inside each model. Maybe is it possible to reuse it for dicts somehow? just and idea

answer from https://stackoverflow.com/a/74769085/8032027:

If you are wondering, whether there is a way to just dynamically infer the TypedDict rather than just duplicating the model fields manually, the answer is no.

Okay :(

@DXsmiley
Copy link
Contributor Author

DXsmiley commented Sep 23, 2023

general quesiton: will it give the ability to pass arbitrary keys to such dicts without problems (i mean without typing errors)? for example user may want to create records with custom fields that are not described in lexicon but could be saved in repo

It won't allow arbitrary keys. I wasn't aware this was a feature (of bluesky) at all. Can you point to an example in the codebase where someone might want to do this?

@MarshalX
Copy link
Owner

MarshalX commented Sep 27, 2023

general quesiton: will it give the ability to pass arbitrary keys to such dicts without problems (i mean without typing errors)? for example user may want to create records with custom fields that are not described in lexicon but could be saved in repo

It won't allow arbitrary keys. I wasn't aware this was a feature (of bluesky) at all. Can you point to an example in the codebase where someone might want to do this?

For example this post record contains custom field https://github.com/MarshalX/atproto/blob/main/tests/models/tests/test_custom_post_record.py

And it was possible to create such record using sdk. Via dict in the args

@MarshalX
Copy link
Owner

MarshalX commented Sep 27, 2023

here is how to create such records:

    import ....


    client = Client()
    client.login(...)

    created_post = client.com.atproto.repo.create_record(
        models.ComAtprotoRepoCreateRecord.Data(
            repo=client.me.did,
            collection=ids.AppBskyFeedPost,
            record={
                'createdAt': client.get_current_time_iso(),
                'text': 'custom record',
                'lol': 'kek',
                'lol2': 'kek2',
            },
        )
    )

    rkey = AtUri.from_str(created_post.uri).rkey
    post = client.com.atproto.repo.get_record(
        {'collection': ids.AppBskyFeedPost, 'repo': client.me.did, 'rkey': rkey}
    )

    print(post)

probably it will work with new changes because of record: 'UnknownType' where UnknownType: te.TypeAlias = t.Union[UnknownRecordTypePydantic, 'dot_dict.DotDictType']

But will pyright raise errors on the provided code above because of DotDictType (not typed dict)?

@DXsmiley
Copy link
Contributor Author

I've added UnknownInputType, which allows plain dicts in places where a user might supply one. Result types are still the same though.

atproto/xrpc_client/models/unknown_type.py Outdated Show resolved Hide resolved
atproto/codegen/models/generator.py Outdated Show resolved Hide resolved
@DXsmiley
Copy link
Contributor Author

done!

@MarshalX MarshalX changed the title Better typing for codegen functions Add TypedDict for params and data arguments; add type hint for kwargs Sep 28, 2023
@MarshalX MarshalX merged commit dab0fd0 into MarshalX:main Sep 28, 2023
6 checks passed
@MarshalX
Copy link
Owner

thank you!!

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