Add global node_id to GHObject + GHTeam extends GHObject#791
Conversation
|
This project is not ready to make backward incompatible API changes. When we do it, it will be a much larger change with a bunch of other clean ups. Please propose how you'd make this change without breaking api compatibility. |
| * | ||
| * @return the id | ||
| */ | ||
| public int getId() { |
There was a problem hiding this comment.
We have a BridgeMethods to handle cases like these. Preserving backward compatibility while changing method return types.
There was a problem hiding this comment.
Seems like it works only at runtime..
IDE still shows errors when trying to use it to return int..
But i assume its ok..
There was a problem hiding this comment.
I'm not sure this is okay. If I had:
int a = team.getId();
Does it compile still with this change?
There was a problem hiding this comment.
incompatible types: possible lossy conversion from long to int
|
@bitwiseman There is also class that have implemented node_id on its own.. |
bitwiseman
left a comment
bitwiseman
left a comment
There was a problem hiding this comment.
Can you do the getNodeId() addition without doing the getId() change?
They seem like distinct changes anyway.
| * | ||
| * @return the id | ||
| */ | ||
| public int getId() { |
There was a problem hiding this comment.
I'm not sure this is okay. If I had:
int a = team.getId();
Does it compile still with this change?
| GHTeam teamByName = organization.getTeams().get("Core Developers"); | ||
|
|
||
| GHTeam teamById = gitHub.getTeam(teamByName.getId()); | ||
| GHTeam teamById = gitHub.getTeam((int) teamByName.getId()); |
There was a problem hiding this comment.
Right... so this means there will be build breaks for clients and we can't have that.
There was a problem hiding this comment.
That would mean we would have not to extend GHObject in GHTeam but write own implementation of node_id.. (And I actually need GHTeam)
And thats actually not good idea (it should extend it from the start)..
Im not sure how bridge methods should work (but in java you cannot have 2 methods with same signature just different return type), but probably it just work at runtime..
At compile time it for sure gives error..
There was a problem hiding this comment.
If you want me to remove extension and implement node id in GHTeam without issues..
There was a problem hiding this comment.
@bitwiseman From code perspective I see that such "breaking" changes was already being mage few times (that changes are binary compatible because of that bridge methods)
And GHObject was also moved from int to long not far away...
#388
So I think its safe enought to do it..
bitwiseman
force-pushed
the
global_node_id
branch
from
fb6acd9 to
87f37e9
Compare
|
Added tests and did some reformatting. |
|
@bitwiseman Thanks.. |
|
Will release the next version in the next week or so. If you have any ideas on supporting GitHub API v4, I'd be interested to hear them. |
|
@bitwiseman To connect to GH v4 API Im using "Allegro" framework (https://github.com/apollographql/apollo-android) Calls itself are rather simple.. For now Im just using GraphQL to:
|
2 participants
Description
That PR is adding Global Node ID according to https://developer.github.com/v4/guides/using-global-node-ids/
Im trying to mix v3 and v4 (graphQL) of GH API together and without Global Node IDs its really hard.
Extending GHObject in GHTeam changed signature of ID, so its back incompatible change (Im not sure why GHTeam was not extending GHObject, in my opinion there is no reason for not doing it).
Please let me know whats best idea to test if it will work on all objects that extends GHObject..
Before submitting a PR:
master. Create your PR from that branch.mvn clean compilelocally. This may reformat your code, commit those changes.mvn -D enable-ci clean install sitelocally. If this command doesn't succeed, your change will not pass CI.