Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

Platform API version now explicit and defaults to v2.0, as mentioned in #33 #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kchr
Copy link

@kchr kchr commented May 7, 2015

Added explicit reference to platform API version, as introduced by Facebook in version 2.0 (Issue #33)

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@psineur
Copy link
Contributor

psineur commented May 15, 2015

Ok, so as I commented on #33 - v1.0 being deprecated doesn't break the fbconsole. Unversioned requests are just being defaulted to v2.0

I think top priority right now will be #37 or related Python 2/3 Unix/Windows bugs that we have that might be fixed with this.

I definitely want to bring better support of versioning to fbconsole, so here's the idea:

  1. Default to last available version 2.3
  2. add version property that will support something like this:
fbconsole.setVersion(2.3) - I like this one cause it's easier to type.
fbconsole.setVersion(2) - shortcut for 2.0
fbconsole.setVersion('2.3') - usually we set versions as strings in other SDKs
fbconsole.setVersion('v2.3')
  1. Allow overwriting version in request path:

    fbconsole.get('/me') should use the set version, but
    fbconsole.get('/v2.1/me') overrides set version to 2.1 for this request

3.1 Fix get('me') failing - just ensure that path starts with slash or use URI lib / better logic there.

What do you think? If this is fine with you and you want to update your pull request - I'm happy to merge it as soon as possible.
If you don't have time for this - it's fine, I guess I will start looking into versioning right after #37, so I guess I will close your issue as needs-no-action now.

@kchr
Copy link
Author

kchr commented May 16, 2015

Hi Stepan!

Thanks for the feedback, that sounds like a better long-term solution, with less impact on the end users who already did the switch in their request URIs (specifying version on their own). I'd be glad to adapt the solution this way; just need a few more days to sort out some other projects.

Expect an update during the upcoming week or so!

And thanks for the API work, keep it up. 👍

-- kchr

@psineur
Copy link
Contributor

psineur commented May 18, 2015

We're going to use Phabricator-style tags for Pull Request status:

  • "review needed" means that reviewer didn't finish reviewing Pull Request yet, or that author doesn't agree with previously requested changes and provided explanation why review needs to be re-done
    • "needs revision" means that changes are needed to proceed
    • "accepted" means pull request is reviewed and can be merged either immediately, after some dependencies and/or after fixing minor nits/issues.

Please change GH Review: needs revision to GH Review: needs review after updating the Pull Requests.

Hope this will not bother people too much.
Cheers!

@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@ghost ghost added the CLA Signed label Jul 12, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants