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

Remote API discussion #2

Open
winston opened this issue Aug 9, 2015 · 4 comments
Open

Remote API discussion #2

winston opened this issue Aug 9, 2015 · 4 comments

Comments

@winston
Copy link
Member

winston commented Aug 9, 2015

Am thinking that from the dependency injection angle (I hope I am using this correctly),
that the remote API might look better like this?

# current

client = Octokit::Client.new(access_token: ENV["OAUTH_TOKEN"])
news = Whatsnew.about client.contents("jollygoodcode/whatsnew")

# proposal

client = Octokit::Client.new(access_token: ENV["OAUTH_TOKEN"])
news = Whatsnew.about "jollygoodcode/whatsnew", client

So the client is passed in as an object. In the future, maybe can extend to BitBucket?
Otherwise the code is right now coupled to Sawyer::Resource.

wdyt?

Great work btw! :)

@choonkeat
Copy link

the client is passed in as an object. In the future, maybe can extend to BitBucket? Otherwise the code is right now coupled to Sawyer::Resource.

Not entirely sure it'll make much difference. Since sawyer is just http lib, bitbucket support is not precluded.

However, one way to make Whatsnew.about agnostic is to expect an interface as argument. If argument responds to find to iterate the content of a repo, then we'd not bother content is in git repo, svn checkout directory, remote github or bitbucket repo, network file share, dropbox...

# instead of passing `path` and performing `Dir.glob` inside, we do it outside
files = Dir.glob(File.join(path, "*".freeze))
news = Whatsnew.about files

# wrap the necessary "raw" data from elsewhere
files = GithubRepoFilesWrapper.new client.contents("jollygoodcode/whatsnew")
news = Whatsnew.about files

@winston
Copy link
Member Author

winston commented Aug 9, 2015

bitbucket support is not precluded.

Ah sure!

agnostic is to expect an interface as argument

I guess this is a design preference for the entry point to a public api.

Usually for utility class like this, I like to have one class method (like this about) that helps with doing the heavy lifting, which means the figuring out of what kind of client/repo can happen in the metho (which is what is happening)

The GithubRepoFilesWrapper then would be an adaptor that the class method eventually wraps around the passed in params and then continue to pass it onto an instance which is agnostic as you have mentioned.

So in this case, the user can use this method very easily like

Whatsnew.about '/name' # local
Whatsnew.about '/name', client # remote and so figure out what adaptor to use in `about`

In this way, then it's also similar to the command line utility which is exposed:

> whatsnew # uses local file directory

@JuanitoFatas
Copy link
Contributor

Thanks for bringing this concern to discussion. 🙇

👍 for the idea of interface as argument.

For network file share, dropbox... not supported for now. Can open issues to KIV.

Instead of passing the octokit client in, let's make it generic that the interface will accept any array of objects which each object respond to certain messages. See project.rb in #3.

@choonkeat
Copy link

I guess this is a design preference for the entry point to a public api.

Yes it is. Also, is a question of scope: where does a utility start; where does it end?

For network file share, dropbox... not supported for now. Can open issues to KIV.

the point is not to support them explicitly. an open interface allows for app-level code to be written without going through here.

If the lib decide to take on responsibility to perform case...when internally, e.g. when Octokit::Client; do act_as_github; when BitBucket ; do act_as_bitbucket; else do act_as_local_dir then the lib will have to march lock step in updates based on those libs, and worse: new libs cannot be incorporated without PR/merge/gem version dance.

imho ruby landscape has too many overreaching libs, and would be better if we have smaller, composable utils that does lesser things.

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

No branches or pull requests

3 participants