Skip to content

Conversation

@S1v4
Copy link
Contributor

@S1v4 S1v4 commented Aug 1, 2017

No description provided.

@S1v4 S1v4 requested a review from claudiofullscreen August 1, 2017 21:22
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling dbc40caa4c742df3a091b01bf2b5742ca86a0938 on insights into d3ccea3 on master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can merge request.run.body here instead of just the response code, but I think this is easier to test or find failed requests.

lib/fb/metric.rb Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find that I need to use symbolize_keys in many places and they're not necessarily a resource. I think we could make a helper module instead.

lib/fb/metric.rb Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I have Day as a private class or just use OpenStruct? Using symbolize_key in combination with parsing end_time is a problem for a OpenStruct one liner.

lib/fb/page.rb Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

format and format_metric should problem go in another class since other resources (e.g. videos) will use it too. It will here for now. The next resources that require insights are posts which is still part of Page.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3182e684c28ebfa406319f815bdc97008a56d0a2 on insights into d3ccea3 on master.

lib/fb/page.rb Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

id.to_sum not needed

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a003e29 on insights into d3ccea3 on master.

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

Successfully merging this pull request may close these issues.

3 participants