Skip to content

Add withXxxxx() methods to AbstractUser #273

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

Merged
merged 5 commits into from
Nov 28, 2018

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