-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client defaults to requests without version param #69
Comments
Hey @octopi - unfortunately I've been a bit short on time lately. I'll see if I can find some time to give this a look over the next week or so. |
Thanks for addressing, but it looks like this commit actually creates a new problem. Instead of picking the current date, you ought to stick to a "minimum supported version" for the library. Versioning is meant to protect against breaking JSON changes we have to make. So for example, today your library will run with v=20131113 and might be fine, but if we make a breaking change tomorrow, your library will run as v=20131114 and the parsing of our new JSON will fail (even though if v were still 20131113, it would have worked). Does this make sense? |
I can definitely see the desire to go this route. My thinking was that:
Individual users can override this default when setting up their client connection, so if they want to pin to a specific version they can without issue (and should). Happy to add something to the docs about this as well. WDYT? |
Hi @mattmueller. Maybe it would be better to make an API version parameter obligatory on class initialisation while giving a choice of specifying a version or automatically picking the latest one? |
Think this would depend on what structure the library is capable of parsing. For example, if the user passes in v=20110601, the venues search response returns a "groups" object at the top-level. Using v=20131113 returns a "venues" object at the top-level. If the library isn't equipped to support the older version, it will break because the user passed in an older version. Allowing v to be an optional param is certainly a good idea, but there should definitely be a "minimum supported version" for the library that gets bumped up every so often. mLewisLogic's Foursquare Python library is a good example of implementing this (mLewisLogic/foursquare@3156812, mLewisLogic/foursquare@5c00832) |
So, 2 approaches I'm going to mull on:
The first option is more future (and past)-proof, the second makes implementing code a bit cleaner and more understandable. Please chime in with any thoughts on this, I'll try to start working on it over the weekend if I can find some time. |
Maybe I'm misunderstanding, but with the first option it sounds like developers who would want to use this gem would still have to do a decent amount of work to get it working? If gem users would actually have to parse the JSON, why wouldn't they just use a raw HTTP library? I'm suggesting the second route. Currently the library serves as a great example of a "black box" Foursquare API solution that works right away and going this way will stick to that. If this library does need to be updated in the future because of versioning issues, my thinking is that this ought to be acceptable. I think the onus on gem users should be to keep all their gems and libraries updated and not to make any design decisions when using them :) |
They wouldn't have to fully parse the JSON, they would just have to potentially walk a longer tree to get the values they want. JSON would automatically be parsed and made in to a mash object to make this easier (as it is now). The main difference is that as it stands, depending on the endpoint, in the current version we'll walk part of that tree for them. That being said I tend to agree with you that the second option is preferable - people have to stay with the times to some extent :) Thanks for the comments. |
Some of the issues I brought up in #40 will play a part in this as well. Most importantly, how can the current test suite (which relies upon snapshotted response fixtures) guarantee that the fixtures we're testing against are valid for any given user-provided version? Probably they can't. I agree that the best route is to pick a minimum supported API version (or a range of versions that are known to work) and draw a line in the sand. Make this change a major version release for the gem so users realize the magnitude of upgrading and the possible breakages. Also, we should move fast on this since Foursquare just sent emails saying versionless API calls will begin returning errors as soon as January 28, 2014. Let me know if there's any way I can help along the way. |
@jerodsanto - I've got a branch up that implements the fallback to a version number when none is defined (although it currently defaults to the current date). It would be really helpful if you could change this fallback to a sane value (maybe Jan, 2013?) and begin regenerating fixture files and updating any needed tests. It's not super difficult work, just work to be done, and unfortunately I'm very short on free time right now. |
@mattmueller I honestly think the constantly-updating-fixtures-to-match-api-version route is untenable over time. Instead, I'd suggest using something like VCR to get actual API responses and cache them for reuse. |
@mattmueller - Has this actually been fixed? Since today I can't use the gem anymore because of foursquare forcing the version being part of every API call. |
@mattmueller - Sorry about the last comment, figured it out. Maybe next time I should check first before screaming "Fire" ;) |
When a client is instantiated without a version param, it defaults to making requests without a version. This has been deprecated behavior for some time and soon the API will stop supporting requests without a version and return an error instead of an actual result. Please take some time to update your library to support this so that other developers using your library have the opportunity to upgrade to a working version.
(It looks like an easy enough fix inside client.rb but when I tried to make the change, a bunch of tests failed with an error about "real HTTP requests")
The text was updated successfully, but these errors were encountered: