Formatting changes by paulruescher · Pull Request #49 · mweibel/facebook.ex

paulruescher · 2017-10-24T06:01:40Z

This is still a work-in-progress, but I've done the majority of the work that needs to be done. I wanted to get what I have so far to at least start getting feedback so I can address when I have some free time.

The mocks that I've wrapped tests in make things quite verbose. I'm v open to feedback if you have a more concise way of doing all that mocking.

Aside from the to-do list below, let me know if there's anything else you'd like to see tackled in this PR. I'm super happy to right some release notes since the changes introduced here are rather breaking :)

Still to do:

wrap object count and stream in mocks. I was having issues w/ my access_token (still don't know what's up), but I have one that works now. Will clean up in the next few days
finish updating documentation. I was unfamiliar with these FB endpoints, so I found it rather difficult to get up and running (no fault of the lib IMO). I'd like to spend some time documenting how to get up and running address in separate PR
update @spec definitions
~~restore logging~~
remove deprecated function
remove # TODO, unnecessary dependencies
~~run through credo. I'm open to not doing this if you'd rather me not impose credos coding style~~ address in separate PR
PR feedback

mweibel · 2017-10-24T06:12:05Z

lib/facebook.ex

+  @spec page_likes(page_id :: integer | String.t, access_token) :: integer
+  def page_likes(page_id, access_token) do
+    fan_count(page_id, access_token)
  end


if we're breaking anyway then let's remove that function

mweibel · 2017-10-24T06:18:16Z

That looks great, thanks for your work!
Aside from the comment I wrote inline, we were discussing about headers for e.g. content-type in #48. What's your opinion on this and could we incorporate that change either with this PR or with the other after merging this? (As we'll break the API anyway that would be the perfect timing)

credo I don't know, really. I see it's doing more than what the new formatter in 1.6 does, right? I.e. it doesn't just do formatting but also pointing out possible issues with the code?

paulruescher · 2017-10-24T06:25:39Z

What's your opinion on this and could we incorporate that change either with this PR or with the other after merging this? (As we'll break the API anyway that would be the perfect timing)

Hmmmm... I'll take a look now and see if I have any thoughts. If you don't hear from me then it's safe to say I just ran out of time tonight.

credo I don't know, really. I see it's doing more than what the new formatter in 1.6 does, right? I.e. it doesn't just do formatting but also pointing out possible issues with the code?

I'd only want it for formatting really, but TBH this isn't a deal-breaker for me. The codebase is small enough still that I generally had no issues with following the style already used. I don't mind doing the work to make it pass, but I also don't want to be intrusive on your project. Your pick :)

Follow-up: the build's breaking, and it looks like there's issues picking up the env vars. Any ideas? I don't think I changed anything to make that break.

EDIT: I'll restore the logging stuff you had in place. I took it out when I was working in graph.ex, but forgot to put it back in.

mweibel · 2017-10-24T06:38:37Z

I'd only want it for formatting really, but TBH this isn't a deal-breaker for me. The codebase is small enough still that I generally had no issues with following the style already used. I don't mind doing the work to make it pass, but I also don't want to be intrusive on your project. Your pick :)

I'm at the moment not doing much elixir (that's also why I don't do much on this library except reviewing code etc). So I'd rather have opinions of people actually using it ;) I.e. you, @jovannypcg @dcarneiro @brinco80 @tfinnell f.e.

Follow-up: the build's breaking, and it looks like there's issues picking up the env vars. Any ideas? I don't think I changed anything to make that break.

I think that's related to it being a PR and secure variables of Travis-CI. I assume it passes on your side? I can run it locally as well to double check.
Also now that the things are mocked.. do we even need those env variables still?

paulruescher · 2017-10-24T06:41:10Z

So I'd rather have opinions of people actually using it ;)

I'd throw a +1 onto that vote. I'll wait to see if anyone else has thoughts.

Also now that the things are mocked.. do we even need those env variables still?

Good question. Probably not 👍

I'll investigate when I have time to dig back into this PR, probably Weds/Thurs.

tfinnell · 2017-10-24T23:37:09Z

While adding the video edges I had experimented with credo for formatting checks and parroty/exvcr for replaying the http calls in our tests, though didn't want to add too much in that particular PR.

credo was really cool and I'd love to see that added to this library. exvcr is also neat as it will generate a bunch of JSON files (or whatever response we need) for http requests that get called inside of it's blocks. If the JSON file is found it will mock the response with the encoded JSON data. Basically, it automates the generation of the mocks you created in test/graph_mock.ex. There are a few cons, such as it could bite us if we're not diligent about refreshing & inspecting captured responses, especially when introducing new edges... maybe not more or less than mocks we roll ourselves, though.

paulruescher · 2017-10-25T05:12:33Z

credo was really cool and I'd love to see that added to this library

I'm happy to do this, but I'll do it in another PR just so I'm not polluting this PR too much.

exvcr would probably have been handy while I was doing tests. I don't really have an opinion wrt mock vs exvcr yet. If you have no objections, I'd say I finish up what I have so far with mock, and then we can crack open a new issue for potentially moving to a different system.

paulruescher · 2017-10-25T07:16:26Z

OK, I think this is good for review now. I want to do a few more things, but I think it's all based around adding in credo, or adding documentation which I'll do separately.

I'm still new-ish to Elixir, so don't be afraid to pick it apart 👍

Should fix #45 and #47

tfinnell

Functionality is testing out as expected in IEX, looks good.

Just noted GraphMock should probably only be defined once.

tfinnell · 2017-10-26T04:24:31Z

test/facebook_test.exs

-    %{"id" => id, "about" => about} = data
-    assert(String.length(about) > 0)
-    assert(id == Integer.to_string(@pageId, 10))
+defmodule Facebook.GraphMock do


This can be merged into the test/graph_mock.ex file

So. I might be doing something wrong, but I was getting the following error when trying to use that module. Do you have any idea what I might be doing wrong? I thought mix test would compile it since it's an .ex file.

Edit: I think I'm just used to Phoenix which I'm pretty sure loads up test files like these.

tfinnell · 2017-10-26T04:31:51Z

.gitignore

 doc
 .DS_Store
+
+# asdf


I don't see why these shouldn't go (I've got an .env as well that I don't want committed), but one tip is to add editor or environment variable files to a global gitignore.

https://help.github.com/articles/ignoring-files/#create-a-global-gitignore has more details

Ahh ok. Is this just to prevent .gitignore pollution?

Yup! It also helps prevent you from accidentally committing stuff and generally helps save you from having to edit .gitignores across projects.

mweibel · 2017-10-26T06:06:37Z

That looks great! Thanks.

I couldn't test it yet, hope I can do so today or tomorrow.

mweibel · 2017-10-28T12:56:50Z

test/facebook_test.exs


-  @appId System.get_env("FBEX_APP_ID")
-  @appSecret System.get_env("FBEX_APP_SECRET")
+  import Facebook.GraphMock


warning: unused import Facebook.GraphMock test/facebook_test.exs:4

I think that import is not needed then?

I fixed that before merging

mweibel · 2017-10-28T13:02:23Z

Err, I merged with squashing but somehow in a wrong way so that GitHub didn't recognize it's actually your PR :/ sorry about that.
Anyway, all is in now. I'll check with #48 what to do there and then I'll release a new version on hex.

paulruescher · 2017-10-31T05:23:48Z

Bummer, oh well not a big deal. Thanks for merging that in 👍

paulruescher added 3 commits October 16, 2017 23:23

Convert function returns to be more idiomatic elixir

35e2a05

Wrap HTTP requests in mocks

3af92af

Start updating documentation

2fadf74

mweibel reviewed Oct 24, 2017

View reviewed changes

paulruescher added 3 commits October 24, 2017 23:53

Finish up mocking tests

9846aea

Remove unnecessary deps

0eda4b4

Update @SPEC definitions

178ec21

paulruescher changed the title ~~[WIP] - Formatting changes~~ Formatting changes Oct 25, 2017

tfinnell reviewed Oct 26, 2017

View reviewed changes

paulruescher added 2 commits October 25, 2017 22:08

line notes: update mock location

dec15e5

line note: remove unnecessary .gitignore rules

5305bfa

mweibel reviewed Oct 28, 2017

View reviewed changes

mweibel closed this Oct 28, 2017

This was referenced Oct 31, 2017

Inconsistent Returns #47

Closed

Method names should be in snake case #45

Closed

paulruescher deleted the formatting-changes branch October 31, 2017 05:23

Conversation

paulruescher commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweibel Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel commented Oct 24, 2017

Uh oh!

paulruescher commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweibel commented Oct 24, 2017

Uh oh!

paulruescher commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

paulruescher commented Oct 25, 2017

Uh oh!

paulruescher commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tfinnell left a comment

Choose a reason for hiding this comment

Uh oh!

tfinnell Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

paulruescher Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tfinnell Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

paulruescher Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

tfinnell Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel commented Oct 26, 2017

Uh oh!

mweibel Oct 28, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel Oct 28, 2017

Choose a reason for hiding this comment

Uh oh!

mweibel commented Oct 28, 2017

Uh oh!

paulruescher commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulruescher commented Oct 24, 2017 •

edited

Loading

paulruescher commented Oct 24, 2017 •

edited

Loading

paulruescher commented Oct 24, 2017 •

edited

Loading

tfinnell commented Oct 24, 2017 •

edited

Loading

paulruescher commented Oct 25, 2017 •

edited

Loading

paulruescher Oct 26, 2017 •

edited

Loading