Replace graph #53

tfinnell · 2017-11-09T07:56:47Z

The main change here is Facebook.Graph is replaced with Facebook.GraphAPI & Facebook.GraphVideoAPI, the HTTP response formatting functions were moved into a separate module, ResponseFormatter. One thing to consider is combining and moving the http request + formatter calls into the existing Facebook.Graph, but I do like having the HTTP library exposed in the main library opposed to in another wrapper as we get a bit more flexibility in configuring requests without having to modify a wrapper module.

I ended up updating a bit of documentation as well since I was basically touching every function as I was doing this. Added some @typedocs and provided a few links here and there. Also added a blurb about contributing to the README.

When we bump to v2.11 of the graph API we may be able to ditch the additional wrapper around the video api as I believe it's all now under the single graph_url. Things still seemed to work as they are on v2.11 when I tested, though.

Last thing, looks like #48 is stagnant, so, if there's no feedback that results in major changes to this PR, I'll start looking into implementing the chat endpoints on top of this. Regardless, appreciate any feedback guys!

paulruescher · 2017-11-09T23:18:41Z

Nice 👍

I just took a quick look and haven't done a deep dive, but one thing sticks out: do we even need hackney anymore?

tfinnell · 2017-11-10T03:07:20Z

Ya, you're right. HTTPoison is pulling it in anyway.

mweibel · 2017-11-10T07:26:05Z

README.md

+We encourage contribution from anyone! If you've got an improvement to the documentation or feature you've implemented, please open a [pull request](https://github.com/mweibel/facebook.ex/pulls).
+This project uses [credo](https://github.com/rrrene/credo) for code analysis. Running `mix credo` will give you a nice output which will tell you if any of the changes you've made aren't consistent with the rest of our codebase.

+The Facebook Graph API is fairly large and as such we're not using every facet of it, so if you're not seeing an edge that is handled, please report an [issue](https://github.com/mweibel/facebook.ex/issues).


report an issue or open a PR to add it?

mweibel · 2017-11-10T07:26:15Z

README.md

-See [API Documentation for facebook.ex](http://hexdocs.pm/facebook/).
+## Contributing
+We encourage contribution from anyone! If you've got an improvement to the documentation or feature you've implemented, please open a [pull request](https://github.com/mweibel/facebook.ex/pulls).
+This project uses [credo](https://github.com/rrrene/credo) for code analysis. Running `mix credo` will give you a nice output which will tell you if any of the changes you've made aren't consistent with the rest of our codebase.


mweibel · 2017-11-10T07:30:32Z

Just had a brief look but so far this looks amazing! Thanks for that!

paulruescher · 2017-11-10T08:50:33Z

lib/facebook/graph_api.ex

+
+  alias Facebook.Config
+
+  def process_url(url), do: Config.graph_url <> url


I have a pattern matching idea for how to combine this with the GraphVideoApi module, I just need a sec to test it. Essentially I think we can pattern match off a binary passed in

paulruescher · 2017-11-10T08:52:51Z

test/graph_mock.ex

+    ]
+  end
+
+  defp request do


so much cleaner than what I had 👍

mweibel · 2017-11-10T17:03:44Z

test/facebook_test.exs

-        end,
-        body: fn(_) -> Facebook.GraphMock.me(:success) end
-      ] do
+      with_mock :hackney, GraphMock.mock_options(


:hackney mock is probably not needed anymore, or is it?

mweibel · 2017-11-10T17:18:48Z

Went a bit more through it and that all seems ok. A bit confused about the hackney mocks there. Is it because HTTPoison uses hackney internally?
Anyway, let me know when you consider this done and I'm more than happy to merge!
If you'd like I'll also make you an owner of the package on hex so you could publish new versions too.

paulruescher · 2017-11-10T09:03:19Z

test/facebook_test.exs

-        end,
-        body: fn(_) -> Facebook.GraphMock.me(:success) end
-      ] do
+      with_mock :hackney, GraphMock.mock_options(


If someone wouldn't mind enlightening me, I actually don't understand how this still works. From my undertanding with_mock takes a module, and then mocks out the methods on that module via the given list. How does with_mock :hackney, still work here?

paulruescher · 2017-11-10T19:10:50Z

Oops, looks I forgot to submit my review last night 🤷‍♀️

tfinnell · 2017-11-10T23:47:35Z

Exactly! HTTPoison is just a wrapper for hackney (just as Facebook.Graph was), which is why this still works.

Now, we could be explicit and go the route of mocking the HTTPoison method functions, but I sorta like the consistency as is.

Since it does seem to raise some wtf? reactions, I've added some documentation to that module 😃

Thanks for the feedback @paulruescher & @mweibel -- I think this is good to go.

@mweibel I'd be happy to help out with version publishing!

paulruescher · 2017-11-10T23:49:45Z

Awesome, I def agree with keeping it for now. I know we talked mocking briefly before, but I've been using this as a guide recently for structuring 3rd party API calls of my own http://blog.plataformatec.com.br/2015/10/mocks-and-explicit-contracts/

Seems to work quite well. Definitely future consideration tho 👍

Awesome work 🎉

tfinnell · 2017-11-11T00:10:58Z

I read that same blog post as I was working on this 😄
I keep bringing up this library, but, exVCR seems to address this mocking issue since the http library it uses is configurable. The reason I'm so keen on it is the amount of transparency it provides into the request/responses that an http library creates -- it becomes a super useful tool for debugging!

mweibel · 2017-11-11T11:10:46Z

Thanks!
Merged it. Can you tell me your hex.pm email address so I can add you? (You can also send it to my email address in my git commits if you don't wanna put it here)

tfinnell added 6 commits November 4, 2017 04:31

Add httpoison

53ae07f

Replace calls to Graph with GraphAPI and GraphVideoAPI

71dd996

Update documentation

5d59d29

Simplify http mocks

3a248d6

Clarify & provide link for getting an access token

ceb4b91

Spread spec to multilines to placate credo

1757d0b

tfinnell added 2 commits November 9, 2017 19:01

Remove hackney from deps

febcdd7

Simplify mock options with helper function

b7d31e9

tfinnell force-pushed the ReplaceGraph branch from 4d3c1dc to b7d31e9 Compare November 10, 2017 03:06

mweibel reviewed Nov 10, 2017

View reviewed changes

tfinnell added 3 commits November 10, 2017 00:08

Add more explicit contribution request

70580d8

Use GraphAPI for Streams & remove Graph

ce4167c

Start pipelines with raw values

4800548

paulruescher reviewed Nov 10, 2017

View reviewed changes

test/graph_mock.ex

]

end

defp request do

Copy link

Collaborator

paulruescher Nov 10, 2017

Choose a reason for hiding this comment

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

so much cleaner than what I had 👍

mweibel reviewed Nov 10, 2017

View reviewed changes

paulruescher reviewed Nov 10, 2017

View reviewed changes

Add documentation for use of with_mock :hackney

2c99d84

mweibel merged commit ec0daaa into mweibel:master Nov 11, 2017


		alias Facebook.Config

		def process_url(url), do: Config.graph_url <> url

Replace graph #53

Replace graph #53

Uh oh!

Conversation

tfinnell commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulruescher commented Nov 9, 2017

Uh oh!

tfinnell commented Nov 10, 2017

Uh oh!

mweibel Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel commented Nov 10, 2017

Uh oh!

paulruescher Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

paulruescher Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel commented Nov 10, 2017

Uh oh!

paulruescher Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

paulruescher commented Nov 10, 2017

Uh oh!

tfinnell commented Nov 10, 2017

Uh oh!

paulruescher commented Nov 10, 2017

Uh oh!

tfinnell commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweibel commented Nov 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tfinnell commented Nov 9, 2017 •

edited

Loading

tfinnell commented Nov 11, 2017 •

edited

Loading