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

Resolver: refactor #10813

Merged
merged 8 commits into from Oct 26, 2023
Merged

Resolver: refactor #10813

merged 8 commits into from Oct 26, 2023

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 10, 2023

Simplify a couple of things, and we can remove the overrides from .com after this.

  • _get_canonical_project and _get_canonical_project_data are basically the same, the former tries to recursively resolve the project for cases that we don't really support, and the second just checks for the cases that we do support and returns the relationship in case of a subproject. So I just renamed _get_canonical_project_data to _get_canonical_project.
  • We were passing the project slug when resolving the path (this was a residual from where we were allowing USE_SUBDOMAIN=False).
  • Resolving is now split into two steps: resolving the domain, and resolving the path.
  • We were using require_https for .com only, this was since on .com we were using the https attribute to track the progress of a custom domain, this is no longer the case, all custom domains on .com are https.
  • _use_custom_domain is the same as _use_cname.
  • Two more methods to resolve a path were added, they are basically the same as resolve, but they work on the object itself, instead of passing each part separately. This results in fewer queries in case the version object is already in memory.

@stsewd stsewd marked this pull request as ready for review October 11, 2023 21:42
@stsewd stsewd requested a review from a team as a code owner October 11, 2023 21:42
@ericholscher
Copy link
Member

Looked at this today but didn't have enough time to fully review this. Will give it a longer look tomorrow, but this looks like a solid win!

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a good refactor 👍

readthedocs/core/resolver.py Show resolved Hide resolved
@stsewd stsewd merged commit 1ac3b46 into main Oct 26, 2023
4 of 5 checks passed
@stsewd stsewd deleted the refactor-resolver branch October 26, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants