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

Would you be open to a bit of refactoring? #92

Closed
gabrieldemarmiesse opened this issue Aug 10, 2023 · 4 comments · May be fixed by #102
Closed

Would you be open to a bit of refactoring? #92

gabrieldemarmiesse opened this issue Aug 10, 2023 · 4 comments · May be fixed by #102

Comments

@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented Aug 10, 2023

To make it easier to add support for keyword arguments #40 , I would like to do some light refactoring. For instance, hide the cache and Function._pending behind getters, so that it's impossible to forget to update it. It would also go in the Resolver class since it's not really the job of Function to cache the result of the resolver.

That would also potentially require some change in some public methods of those classes defined in this part of the doc: https://beartype.github.io/plum/api.html I wonder if we can change the api of those function without requiring a deprecation procedure (warning, backward compatibility, etc). Of course methods and functions described in "basic usage" and "advanced usage" should be considered intouchable.

If you're ok with some light refactoring I'll do several small PRs to make it easier to discuss the changes

@gabrieldemarmiesse gabrieldemarmiesse changed the title Would you be open to a few refactoring? Would you be open to a bit of refactoring? Aug 12, 2023
@wesselb
Copy link
Member

wesselb commented Aug 13, 2023

Hey @gabrieldemarmiesse! As long as the API changes are mostly changes to internals, not API changes to the more commonly used functions, then I'd definitely be open to refactoring. :) With regards to moving things around, e.g. moving _pending to Resolver, I'd be open to that too, as long as the changes don't significantly increase coupling: the resolver current doesn't hold a reference to the function, and it would be nice to keep it that way.

@gabrieldemarmiesse
Copy link
Contributor Author

Sure thing, that would be possible

@gabrieldemarmiesse
Copy link
Contributor Author

gabrieldemarmiesse commented Sep 4, 2023

Hi @wesselb, I wanted to give you an update on my work. I made the refactoring I had in mind and it helped me a lot to focus on the issue of keyword arguments. Now that it's done, it's actually hard to make the keyword argument system work, because of how flexible and fast plum is (caching is important here).

I kind of gave up at this point (sorry I was young and full of dreams).

I tried to focus more on the @overload use case and forget about performance, and I tried to make a lib that does the dispatch only on implementations supported by @overload. Suprisingly, I had quite some success by matching the behavior of mypy and pyright.

So I kinda made a competitor of plum (but not really since my lib does not support crazy stuff) in the process, sorry again :( I hope that both projects can learn from each other. -> https://github.com/gabrieldemarmiesse/overtake

If it goes well, my goal is to make a PEP so that @overload can support multiple dispatch via third party libraries. Currently, we're in a grey area, hopefully we can do something like https://peps.python.org/pep-0681/ where we declare a "multiple dispatch" library and the type checkers will behave accordingly.

@wesselb
Copy link
Member

wesselb commented Sep 9, 2023

Hey @gabrieldemarmiesse! Sorry for the delay in replying. I've just got back to work. :)

First of all, the PRs that you've put together thus far are super helpful. Your efforts really are much appreciated! It's a shame to hear that the keyword argument system is more complicated than anticipated, but I suppose that's how it is. :(

Congrats on putting overtake together!! Obviously there is no need to apologise. I think that it super cool to see that you've come up with a solution that works for you! Perhaps parts of overtake could be merged in Plum or the other way around in the future. :)

I see that you've made a draft for a PEP on the other thread, so I'll reply there about that. Again, I really appreciate your contributions, and think it's exciting that you've started overtake!

@wesselb wesselb closed this as completed Sep 23, 2023
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 a pull request may close this issue.

2 participants