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

Add a facade for all Github HTTP interactions #578

Closed
nateberkopec opened this Issue Jul 3, 2017 · 1 comment

Comments

Projects
None yet
1 participant
@nateberkopec
Collaborator

nateberkopec commented Jul 3, 2017

Githubbub stuff kind of litters the codebase r/n. There should be a Remote Facade.

@nateberkopec

This comment has been minimized.

Show comment
Hide comment
@nateberkopec

nateberkopec Jul 3, 2017

Collaborator

I.e. the only place we should be rescuing from Githubbub::RequestError is in the facade object. One has already been started in GithubFetcher, but it needs to be fleshed out.

As an example, here's what populate_docs! would look like with a proper remote facade:

def fetcher
  @fetcher ||= GithubFetcher.new(full_name)
end

def populate_docs!
    return unless can_doctor_docs?
    
    self.update!(commit_sha: fetcher.commit_sha) # Should never raise Githubbub::RequestError
    parser  = Repo::CLASS_FOR_DOC_LANGUAGE[self.language].new(fetcher.clone) # ditto, but deal with the nil case here too
    parser.process
    parser.store(self)
  end
Collaborator

nateberkopec commented Jul 3, 2017

I.e. the only place we should be rescuing from Githubbub::RequestError is in the facade object. One has already been started in GithubFetcher, but it needs to be fleshed out.

As an example, here's what populate_docs! would look like with a proper remote facade:

def fetcher
  @fetcher ||= GithubFetcher.new(full_name)
end

def populate_docs!
    return unless can_doctor_docs?
    
    self.update!(commit_sha: fetcher.commit_sha) # Should never raise Githubbub::RequestError
    parser  = Repo::CLASS_FOR_DOC_LANGUAGE[self.language].new(fetcher.clone) # ditto, but deal with the nil case here too
    parser.process
    parser.store(self)
  end

DavidRagone added a commit to DavidRagone/codetriage that referenced this issue Jul 7, 2017

Add a facade for all Github HTTP interactions
codetriage#578

* Extract all Repo-related GitHubBub code into a facade
* Group (future) facades into GithubFetcher module (in app/models)
* Introduce new service for populating issues (using perhaps too fancy
`Module.()` pattern to call `call()` method)
* Use response classes and null object pattern to unify `#issues` interface to single duck

Dearly missing tests

DavidRagone added a commit to DavidRagone/codetriage that referenced this issue Jul 7, 2017

Add a facade for all Github HTTP interactions
codetriage#578

* Extract all Repo-related GitHubBub code into a facade
* Group (future) facades into GithubFetcher module (in app/models)
* Introduce new service for populating issues (using perhaps too fancy
`Module.()` pattern to call `call()` method)
* Use response classes and null object pattern to unify `#issues` interface to single duck

Dearly missing tests

@schneems schneems closed this in #590 Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment