-
Notifications
You must be signed in to change notification settings - Fork 109
Ensure @Value annotation on immutable classes #46
Conversation
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.
I think replacing some code caused the API to change slightly.
@@ -10,6 +12,8 @@ | |||
/** | |||
* The auction state. | |||
*/ | |||
@Value | |||
@Wither |
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.
I think this @Wither
added more methods than there where originally. Before this PR, only withStatus
was available.
|
||
/** | ||
* | ||
*/ | ||
@Value | ||
public class Query { |
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.
The classes modelling the API to ElasticSearch don't always need the @Value
annotation.
@@ -1,11 +1,13 @@ | |||
package com.example.auction.transaction.api; | |||
|
|||
import java.util.UUID; | |||
import lombok.Value; | |||
|
|||
public abstract class TransactionEvent { | |||
|
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.
All the code in transaction-xxx
is WIP and it'd be good to not add more complexity on it until we have a clear idea what is it we want to do with it.
|
||
@Value | ||
public final class TransactionMessage { |
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.
See comment above
|
||
@Value | ||
public final class TransactionInfo { |
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.
See comment above
This clashes with #52 |
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.
Thanks, @yg-apaza. Just a couple of fixes to make to the whitespace formatting.
@@ -5,7 +5,7 @@ | |||
public abstract class TransactionEvent { | |||
|
|||
private TransactionEvent() {} | |||
|
|||
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.
It looks like some extraneous whitespace changes snuck in.
Here are some instructions for configuring Eclipse to strip trailing whitespace on save: http://stackoverflow.com/a/14178860
|
||
import java.util.Optional; | ||
|
||
public interface UserCommand extends Jsonable { | ||
@Value |
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.
It looks like some files are indenting with tab characters instead of space characters.
Here are instructions for configuring Eclipse to use spaces instead of tabs: http://stackoverflow.com/a/408449
😅 I forgot to configure properly formatting in Eclipse, now I fixed it. |
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.
Thank you!
@ignasi35 I'll let you have one last look before merging |
libraryDependencies += lagomJavadslApi | ||
libraryDependencies ++= Seq( | ||
lagomJavadslApi, | ||
lombok |
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.
I think this dependency is not used ATM.
Fixes #9