-
Notifications
You must be signed in to change notification settings - Fork 176
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
Adding all Parameter for ClientCoreProfile factory #28
Conversation
Hi Steven, Thanks for your PR, it looks good. There's some problems with the CI. I'll investigate later.
|
Hi Thomas, I made another commit I would like to add. I implemented client side of Local Auth List Steven |
Hi Steven, Wow, thanks allot for your contribution. Thomas |
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.
Some fine work, follows the general design/naming and is well tested. I have commented two places that needs to be addressed before I can merge (since it's breaking the build).
@@ -187,11 +187,12 @@ public MeterValuesRequest createMeterValuesRequest(Integer connectorId, MeterVal | |||
* @see StartTransactionRequest | |||
* @see StartTransactionFeature | |||
*/ | |||
public StartTransactionRequest createStartTransactionRequest(Integer connectorId, String idTag, Integer meterStart, Calendar timestamp) throws PropertyConstraintException { | |||
public StartTransactionRequest createStartTransactionRequest(Integer connectorId, String idTag, Integer meterStart, int reservationId, Calendar timestamp) throws PropertyConstraintException { |
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 idea with the create metodes on the profile, has to help make a request with all the mandatory fields set.
The field reservatopmID wasn't a part of this method, since it's not mandatory.
I'd recommend removing it from this method and setting it on the return object locally.
@@ -223,10 +224,11 @@ public StatusNotificationRequest createStatusNotificationRequest(Integer connect | |||
* @param transactionId required. The identification of the transaction. | |||
* @return an instance of {@link StopTransactionRequest}. | |||
*/ | |||
public StopTransactionRequest createStopTransactionRequest(int meterStop, Calendar timestamp, int transactionId) { | |||
public StopTransactionRequest createStopTransactionRequest(int meterStop, Calendar timestamp, Reason reason, int transactionId) { |
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.
Reason wasn't part of the parameters since it's not a mandatory field. See explanation above.
…arameter aren't manditory
Done, hopefully the CI will build now. |
Looks like it 😀 |
Also cleaned up if doesn't connect, it doesn't show the call stack every time