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 typehints to Live's public methods #770

Merged
merged 7 commits into from
Feb 7, 2024
Merged

Conversation

AlexandreKempf
Copy link
Contributor

@AlexandreKempf AlexandreKempf commented Feb 5, 2024

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@AlexandreKempf
Copy link
Contributor Author

@dberenbaum Thanks for the feedback. I'll take your suggestions into account.
Meanwhile, I have a broader question on type hints. I agree that the current implementation for StrPath or ParamLike is cleaner the current way. But when an editor displays the help for each function, it is not very informative at first sight to have a StrPath type as a user, and I would rather have a str | pathlib.Path. Do you know a way to have the best of both worlds?

@dberenbaum
Copy link
Collaborator

Do you know a way to have the best of both worlds?

πŸ€” Good point. I don't know a way, but might be worth a little research. If you think it's helpful to have the more explicit types, I'm fine to keep them that way.

- Removes redundancy with types import
- strPath is now using PurePath instead of Path
@AlexandreKempf
Copy link
Contributor Author

I asked the question to tech people; we'll see.

I believe it will take more effort to maintain, but it will be way more user-friendly to have a list of possible plots like ["linear," "simple, ...] for the user to play with and discover the tool. Rather than an opaque [*BASE_SCHEMA.schema["type"].validators]. (I took this one from the studio-event data format).

You see my point, I guess, we just need to figure out own much a pain it will be to maintain to know if it is worth it :)

@dberenbaum
Copy link
Collaborator

As mentioned by @jonburdo, I take back importing from elsewhere for type checking. It seems like an anti-pattern with more thought, and we are better off listing the choices or probably even just leaving the type hints more general for now.

@AlexandreKempf
Copy link
Contributor Author

I agree with you two after the explanation from @jonburdo. I still believe the user-facing functions should have a clear and readable type from the editor. But it is not an urgent matter, and we can always get back at it later.

@dberenbaum
Copy link
Collaborator

@AlexandreKempf Could you check the CI failures?

@jonburdo
Copy link

jonburdo commented Feb 6, 2024

I believe it will take more effort to maintain, but it will be way more user-friendly to have a list of possible plots like ["linear," "simple, ...] for the user to play with and discover the tool.

I think you're right for a relatively short list like this. I sometimes take a look at other popular libraries for questions like this. Running rg '\bLiteral\[' (ripgrep) in the numpy and typeshed repos shows they generally just repeat the whole thing rather than using a variable. I imagine it won't be too hard to maintain.

@AlexandreKempf
Copy link
Contributor Author

Thanks for the advice @jonburdo. What do you think about that @dberenbaum?
I would say we go with Jon's advice here: our lists are small and useful for users for editors' suggestions. If it is too hard to maintain, we can always revert it at no cost :)

@dberenbaum
Copy link
Collaborator

Yes, I don't think it's worth worrying about the maintenance costs at this point

@dberenbaum dberenbaum merged commit b266b80 into main Feb 7, 2024
10 of 11 checks passed
@dberenbaum dberenbaum deleted the add-typehints-to-live branch February 7, 2024 13:51
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.

3 participants