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

WIP: Add StructuredDict for heterogeneous dicts #124

Closed
wants to merge 2 commits into from

Conversation

devdoomari3
Copy link

related: #105

this pull-request adds StructuredDict to allow users to signal their intention to have a dict 'structurally typed'

ps: this is my first commit on MonkeyType, so I'd really appreciate some feedbacks (hence WIP)

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@carljm
Copy link
Contributor

carljm commented Dec 5, 2018

Hi @devdoomari3 ! Thanks very much for the idea and the pull request, but I don't think we want to take this approach to the problem. Switching your code to use a subclass of dict at runtime can be risky (exact type equality checks that passed before could fail, e.g. if using msgpack with strict types, performance can suffer), and I don't think there's a need for it here. The goal of MonkeyType is to generate best-effort annotations without requiring changes to your code. By the time you've gone and figured out which dicts in your code should be StructuredDict, you may as well just go ahead and create the TypedDict type and annotate using it. The other problem is that if you use StructuredDict, then let MonkeyType annotate as a TypedDict, you'll immediately get typecheck errors after annotating because the typechecker won't consider StructuredDict a compatible type with the TypedDict annotation.

I think we can get pretty useful behavior with heuristics as described in #105 (can make the size threshold for storing per-key types in tracing customizable, and people can use TypeRewriters to customize the output heuristics to their satisfaction), and we should go that route.

If you're interested in updating this PR to take that approach, that would be great! Let me know and I'll reopen.

@carljm carljm closed this Dec 5, 2018
@devdoomari3
Copy link
Author

devdoomari3 commented Dec 5, 2018

@carljm I was thinking about replacing all StructuredDict to TypedDict after running Monkeytype.
(so after running MonkeyType, we won't see StructuredDict on the source file)

would it be ok then?
Also, would it take long to implement heuristics? (if this is rejected)

  • would comments that show intention to use TypedDict work? (if heuristics take too long)
// @monkeytype-typed-dict    <-- can these comments be visible on monkey-type?
{
   'users': {
      'user1': User( ... ),
   },
   // @monkeytype-typed-dict
   'settings': {
      'theme': Themes.DarkMode,
      ...
   }
}

(background:
I really need MonkeyType to work with heterogeneous types because my current company returns a lot of un-typed dictionaries on API, and changes made on them leak to frontend...

So my plan was to get MonkeyType implement heterogeneous types generation, and use https://github.com/devdoomari3/python-codegen to generate Typescript API code (with CI updating TS api code for every commit on server side)

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.

None yet

3 participants