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

🔧 Group logic concerning the cache in one single method #96

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented Aug 17, 2023

  • The cache is read and updated every time the method Function.resolve_method is called. This was done in various places in the code, making it hard to understand and easy to get wrong. Now one single method is in charge of reading and updating the cache: _resolve_method_with_cache.
  • This function also ensures there are no pending registration, so it's not necessary to manually call self._resolve_pending_registrations() every time before resolving the method.
  • The resolved_method has been made shorter by extracting the logic concerning the __mro__ in another method. This allows the method resolved_method to have less local variables overall and less variables that are only used in specific situations.

Later on, it will be possible to move the cache into the resolver, but we need to start somewhere, the aim of this PR is just to make the code clearer, not to have the perfect structure

Related to #92 the goal overall is to have a good structure to add support for keyword arguments, as mentionned in #40

@coveralls
Copy link

coveralls commented Aug 17, 2023

Pull Request Test Coverage Report for Build 5902467618

  • 37 of 37 (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 5894385101: 0.0%
Covered Lines: 927
Relevant Lines: 927

💛 - Coveralls

Copy link
Collaborator

@PhilipVinc PhilipVinc left a comment

Choose a reason for hiding this comment

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

@wesselb This looks like a nice refactoring, and I support it.

(Wessel has been trying to keep perfect coverage, so maybe you should fix that one line that is no longer covered by tests?)

@gabrieldemarmiesse
Copy link
Contributor Author

Thanks for the review @PhilipVinc , I added a small unit test for the missing coverage

@wesselb
Copy link
Member

wesselb commented Aug 19, 2023

@gabrieldemarmiesse Thanks for the refactoring. I think this is a lot more clear and a definite improvement. Merging right away :)

@wesselb wesselb merged commit c10fc47 into beartype:master Aug 19, 2023
6 checks passed
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.

4 participants