Skip to content
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

Get all invitees to a specific repository #1513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Get all invitees to a specific repository #1513

wants to merge 2 commits into from

Conversation

ShalomGottesman
Copy link
Contributor

@ShalomGottesman ShalomGottesman commented Jan 19, 2020

Changes:

  • Repo: add method to fetch all the invitees.
  • User: add method to fetch invitations to a repo and method to accept invitations.

@0crat 0crat added the scope label Jan 19, 2020
@0crat
Copy link

0crat commented Jan 19, 2020

Job #1513 is now in scope, role is REV

Copy link
Member

@amihaiemil amihaiemil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman Please see my comments :)

Also, for future reference, please make only one PR/ topic. This PR contains changes in both Repo and User -- there should be 2 separate PRs. But we leave it like this for now :D


public Iterable<String> invitees() throws IOException {
Iterator<JsonValue> iter = this.request.uri().path("/invitations").back().method(Request.GET)
.body().set(Json.createArrayBuilder().build()).back()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman Why do you specify an empty Json body? I am pretty sure it is not needed :)

@Override
public Iterable<Coordinates> invitations() throws IOException {
Iterator<JsonValue> iter = this.github().entry().uri().path("/user/repository_invitations").back().method(Request.GET)
.body().set(Json.createArrayBuilder().build()).back()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman Same here, why the empty Json body?

String repoName = obj.getString("name");
String owner = obj.getJsonObject("owner").getString("login");
Coordinates coords = new Coordinates.Simple(owner, repoName);
coordsSet.add(coords);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman I think you can simplify these 4 lines to just:

coordsSet.add(
    new Coordinates.Simple(
        obj.getString("name"),
        obj.getJsonObject("owner").getString("login")
    )
);

@@ -169,4 +170,14 @@ private String xpath() {
return String.format("/github/users/user[login='%s']", this.self);
}

@Override
public Iterable<Coordinates> invitations() throws IOException {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman Can you also specify "Not yet implemented."?


@Override
public boolean acceptInvitation(Coordinates coords) throws IOException {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman same here pls

}

@Override
public boolean acceptInvitation(Coordinates coords) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman Can you make coorde final? Generally, it's good to work only with final variables and parameters.

Final variables are immutable: this means that an object with immutable fields is thread-safe by default and also a method that works only with final variables will be easier to understand, there will be only one initialization per variable :)

return coordsSet;
}

public boolean acceptInvitation(Coordinates coords) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShalomGottesman You forgot the @Override annotation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants