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

Simplify updates of resolver #97

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented Aug 19, 2023

This PR aims to make sure we don't forget to register any method before accessing the cache or the resolver.
We do so by using getters and setters. I had to change a bit the __repr__ because listing methods from the resolver now automatically register all methods.

It's now nearly impossible to access an outdated cache or resolver.

The function to get the __doc__ is currently a bit weird and do some direct access to the resolver, but I have stuff in my fork to make it cleaner in future PRs

To give a bit more info about where I am now in my fork, I made a new class MethodRegistry in charge of holding the list of methods and destroy/recreate the resolver and cache if a new method is registered. The cache moved to the resolver and the doc generation is the job of the the MethodRegistry. Also the Resolver is "frozen" once created, making it easier to understand the logic around it. The current paradigm about two _pending and _resolved is removed

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5917285022

  • 23 of 23 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 5912676390: 0.0%
Covered Lines: 947
Relevant Lines: 947

💛 - Coveralls

@wesselb
Copy link
Member

wesselb commented Aug 21, 2023

Hey @gabrieldemarmiesse! Thanks for submitting yet another PR. :)

Whereas I think your proposed changes are eminently sensible from a design point-of-view, I’m slightly hesitant to change the point at which methods are resolved.

To properly deal with forward references, promised types, and other sorts of type constructions that one preferable should not be doing, it is important that the methods are always resolved at the very last moment, i.e. when the function is called for the first time. I’m worried that calling the resolution process in getters might inadvertently change the order of operations in more complex designs. Therefore, for the particular operation of registering the methods, I think I might prefer the design without getters where the registration process is called explicitly.

I’m aware that the unit tests still pass with this change, so there shouldn’t be a problem there. I’m more talking about other packages I’m working on that very intensely use Plum, where I’ve encountered a fair share of dispatch-related problems. It has taken quite some effort to get the order of operations just right.

In addition, I think being able to see how many methods are registered and pending is quite a useful feature, especially in dispatch-related problems.

@gabrieldemarmiesse
Copy link
Contributor Author

Thanks for the feedback, I'll close this pull request then

@wesselb
Copy link
Member

wesselb commented Aug 22, 2023

Thanks for your understanding! I do really appreciate your efforts in trying to improve the internal design. :)

@gabrieldemarmiesse gabrieldemarmiesse deleted the simplify_updates_of_resolver branch August 23, 2023 20:43
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