(refactor): RequirementsResolver should have only one method resolve()#949
(refactor): RequirementsResolver should have only one method resolve()#949rd4398 wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
This commits attempts to refactor the RequirementsResolver to have only one method resolve() instead of separate methods It is consistent with existing code in resolver.py where there is only single method for resolution Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
| # Delegate to RequirementResolver (handles caching internally) | ||
| # Delegate to RequirementResolver | ||
| parent_req = self.why[-1][1] if self.why else None | ||
| pbi = self.ctx.package_build_info(req) |
There was a problem hiding this comment.
It seems like this step to get the pbi could move inside the resolver, too.
| if cached_resolution and not req.url: | ||
| wheel_url, resolved_version = cached_resolution | ||
| url, resolved_version = cached_resolution | ||
| logger.debug(f"resolved from previous bootstrap to {resolved_version}") |
There was a problem hiding this comment.
I think the logic of this function would be clearer if you returned early in any cases where you have enough information. So for example here you could just return. Then the else on the next line is not needed and the rest of the code can be moved left 1 indentation level.
| ) | ||
| url, resolved_version = wheels.resolve_prebuilt_wheel( | ||
| ctx=self.ctx, req=req, wheel_server_urls=servers, req_type=req_type | ||
| ) |
There was a problem hiding this comment.
Same here, if we get here and have a result returning means you don't need the else on the next line.
| # Cache the result | ||
| result = (wheel_url, resolved_version) | ||
| result = (url, resolved_version) | ||
| self.cache_resolution(req, result) |
There was a problem hiding this comment.
Making those other structural changes could mean modifying the cache in multiple places. To void that, you could have resolve() look in the cache, call another method if it doesn't find something, then save the results of the other method to the cache before returning. Then all of the more complex logic for dealing with the resolution is in that _resolve() (for example) method.
2 participants
This commits attempts to refactor the RequirementsResolver to have only one method resolve() instead of separate methods
It is consistent with existing code in resolver.py where there is only single method for resolution
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com