-
Notifications
You must be signed in to change notification settings - Fork 805
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
Use global API instances #943
Use global API instances #943
Conversation
Codecov Report
@@ Coverage Diff @@
## master #943 +/- ##
==========================================
- Coverage 94.95% 94.90% -0.06%
==========================================
Files 210 212 +2
Lines 8571 8676 +105
Branches 772 791 +19
==========================================
+ Hits 8139 8234 +95
- Misses 432 442 +10
|
82f13d5
to
94d557b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, i have just one question concerning the api version for each api, how do we bump them ? For each major ?
We bump them whenever we make a change that is not backwards compatible. So adding a method would not require a version bump, as an old version of the API module would simply not call that method. Removing a method that may be called by an older version of the API module would require a version bump. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few comments.
This is just a suggestion as I don't have the whole view on the spec. But few things are repeated (setting, getting, disabling) maybe it makes sense to make some helper function / functions or class for that so the code can be reused and tested in one place ? The difference is only with global key and the returned type, so there could be some similarities. Are they going to be more globals in future too, which should be implemented the same way then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thx for those changes
@mayurkale22 would appreciate if you can take another look at this |
As discussed in the SIG and on issue #885, this uses the
global
object to store references to the global APIs. When an API method is called, it tries to get the global API instance by calling a method with its API version. If the version matches, the call is properly proxied through to the API, else it calls a no-op version.