Configuration documentation #57
Conversation
Added configuration documentation
|
|
||
| ## Configuration | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Can you somehow mention what the default configuration is and where to put that file?
There was a problem hiding this comment.
Oh, that's for Phoenix, sorry... will update
|
Is this ok now @mweibel ? I'm preparing new PRs at the moment ;) |
| graph_url: "https://graph.facebook.com/v2.9", | ||
| graph_video_url: "https://graph-video.facebook.com/v2.9", | ||
| graph_url: "https://graph.facebook.com", | ||
| graph_video_url: "https://graph-video.facebook.com", |
There was a problem hiding this comment.
Should we really have no version as the default configured anymore? We can increase it certainly but IMO having a sensible default seems preferable.
Also this change makes this PR a breaking change, not just a documentation change ;)
There was a problem hiding this comment.
I think it's the right way for a library to leave (or better: force) the choice of the external api version to the user. Else we would be responsible for keeping it up to date with facebook and every update to the library could introduce incompatibilities for people who rely on the version set by the library. I'd rather have the library have no default at all (and raise if the user doesn't set any url), but I think an "eternal" url (which we don't ever need to change) is also ok in terms of easy entry to the library. True, this became more than a documentation change, sorry. ;) Your choice, you can also leave these url changes out of the merge. :)
There was a problem hiding this comment.
We have a breaking change in #58 as well, so we may as well just increase the major version after merging those two PRs.
I'd like you to next time not mix in unrelated changes to a PR (also in #58 changing the function name of set_appsecret() is such a case). Those changes can be made, but making them in a separate PR is better for visibility and to be able to accept/decline the PRs separately.
2 participants
Added configuration documentation