Repository navigation
Conversation
mre
changed the title
a424ad5 to
fe75bd7
Compare
|
Just some quick thoughts.. Yeah, I think this is the right approach. Thanks for getting started! I think we should centralise around the GithubUrl enum. The other files like api.rs are probably less important in a first pass. There is a architecture decision of how to hook it into the website checker. I don't love the before+after functions. Maybe a GithubChecker should wrap a WebsiteChecker, so then the github checker naturally has control over whether to delegate to the website checker. I also see that github/request.rs does some rewriting and logging. I'm increasingly thinking that we need to abstract the remaps/redirect history feature into a generic "url transformation history". It would be something like a But these are just very rough thoughts and it doesn't have to happen all at once (or at all). I'll let you keep working on it and exploring the solution space :) |
thomas-zahner
left a comment
thomas-zahner
left a comment
There was a problem hiding this comment.
I really like this draft already. I also really like how this enables us to simply drop the dependency on octocrab
| &self, | ||
| owner: &str, | ||
| repo: &str, | ||
| ref_: Option<&str>, |
There was a problem hiding this comment.
Is the trailing underscore intentional? Seems to repeat itself quite a lot in this PR.
|
|
||
| #[derive(Debug)] | ||
| pub(crate) struct RepoStatus { | ||
| pub(crate) status: Status, |
There was a problem hiding this comment.
Some doc comments would help here
| /// | ||
| /// This keeps private repository checks working without leaving GitHub API | ||
| /// details in the generic website checker. | ||
| pub(crate) async fn check_after_website_failure(&self, status: Status, uri: &Uri) -> Status { |
There was a problem hiding this comment.
As @katrinafyi mentions the before/after distinction feels a bit funky. But I'm not sure what the best approach is. Making the GitHub checker wrap a Websitechecker might indeed make sense.
fe75bd7 to
2302d50
Compare
3 participants
This PR moves GitHub-specific link handling out of the generic website checker and request quirks.
What changed:
checker::github::url.checker::github::apiand the existingHostPool.checker::github::requestinstead of genericQuirks.WebsiteCheckerdelegates GitHub behavior before and after the normal website path.types::uri::github::GithubUriparser andOctocrabdependency are gone.This is still a draft. I mainly want to check whether this module split feels right before polishing the edge cases further.