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: Fix type-annotate action for functions #1048

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

3Rafal
Copy link
Collaborator

@3Rafal 3Rafal commented Mar 10, 2023

As described in #801

@3Rafal 3Rafal force-pushed the fix-type-annotate-for-functions branch from 439f004 to e50147f Compare March 10, 2023 13:44
let+ original_text = get_source_text doc loc in
let arrow = " -> " in
let typ' =
Str.split (Str.regexp arrow) typ
Copy link
Member

Choose a reason for hiding this comment

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

Str isn't thread safe so we avoid it. Try using Re.split or something similar.

Copy link
Member

Choose a reason for hiding this comment

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

It's also not ideal that we need to deal with strings here at all btw. Shouldn't it be possible to use the type tree representation to drop the args instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey Rudi, it's just a draft and work in progress. I have opened a PR so that it's visible that I'm working on this feature.
Thanks for your suggestions (will keep them in mind), but I'm not sure if it's worth it to do review now. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing. Some people like the feedback early, others prefer it all at the end. I can go either way

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