Skip to content

Conversation

@mdeknowis
Copy link
Contributor

As discussed in #272 adding the missing with* methods to AbstractUsers

@gmessner
Copy link
Collaborator

@mdeknowis
Reviewing this pull request made me realize 2 things:

  1. AbstractUser is not marked abstract, and it should be.
  2. the withXxxx() methods do not work on a base class. The withXxxx() methods are meant to be chained together, which will not work on the sub-classes of AbstractUser. They can be chained but cannot be assigned to a sub-class without casting. This is why the User class has implemented the withXxxx() methods.

An example trying to use with a sub-class:

Assignee assignee = new Assignee()
    .withAvatarUrl("https://asdad.com/qqwwe")
    .withBio("https://wqeqwqew.com/qweqwe");

Will not compile unless you add a type cast as follows:

Assignee assignee = (Assignee) new Assignee()
    .withAvatarUrl("https://asdad.com/qqwwe")
    .withBio("https://wqeqwqew.com/qweqwe");

What sub-class were you trying to use when you needed the withXxxx() methods?

@mdeknowis
Copy link
Contributor Author

@gmessner I adjusted the pull request according to your proposal

Copy link
Collaborator

@gmessner gmessner left a comment

Choose a reason for hiding this comment

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

@mdeknowis
Sorry about the delay getting to this. Been quite busy the last couple weeks. It's too bad that the Java compiler requires the @SuppressWarnings, but it is worth having this feature so I don't mind them.

Just a couple questions that need to be answered and I'll approve and merge. Thanks again for your contribution.

@gmessner
Copy link
Collaborator

@mdeknowis
BTW, As soon as we get this merged, I will also merge the changes that add toString() implementations as per Issue #271

@gmessner gmessner changed the title Add with* to AbstractUser Add withXxxxx() methods to AbstractUser Nov 28, 2018
@gmessner gmessner merged commit cab7f05 into gitlab4j:master Nov 28, 2018
@gmessner
Copy link
Collaborator

@mdeknowis
Thanks again for the contribution. I'll let you know when it is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants