-
Notifications
You must be signed in to change notification settings - Fork 171
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
[0255] Enchance body information #1583
[0255] Enchance body information #1583
Conversation
@santanamk ready for the Ford review |
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.
@KostyaBoss I have reviewed this PR. Code looks good. I have suggested a few changes. Please review, and make the changes as needed.
@@ -31,7 +31,7 @@ public boolean onCreateOptionsMenu(Menu menu) { | |||
|
|||
@Override | |||
public boolean onOptionsItemSelected(MenuItem item) { | |||
// Handle action bar item clicks here. The action bar will | |||
// Handle action bar item clicksere. The action bar will |
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.
@KostyaBoss you can revert this change. No changes are needed in MainActivity.java
.
|
||
import com.smartdevicelink.marshal.JsonRPCMarshaller; | ||
import com.smartdevicelink.proxy.rpc.DoorStatus; | ||
import com.smartdevicelink.proxy.rpc.GateStatus; |
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.
@KostyaBoss You can remove the unused import GateStatus
on line 5.
@Override | ||
public void setUp() { | ||
msg = new DoorStatus(TestValues.GENERAL_GRID, TestValues.GENERAL_DOOR_STATUS_TYPE); | ||
msg.setStatus(TestValues.GENERAL_DOOR_STATUS_TYPE); |
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.
@KostyaBoss You can remove line 27 msg.setStatus(...)
since you are already setting the status
using the DoorStatus
constructor.
DoorStatusType status = msg.getStatus(); | ||
|
||
// Valid Tests | ||
assertTrue(Validator.validateGrid(msg.getLocation(), location)); |
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.
@KostyaBoss I would delete line 36 and replace it with
assertEquals(TestValues.MATCH, TestValues.GENERAL_GRID, location);
@@ -0,0 +1,66 @@ | |||
package com.smartdevicelink.test.rpc.datatypes; |
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.
@KostyaBoss I would rename this file to GateStatusTests.java
.
// Valid Tests | ||
assertTrue(Validator.validateGrid(msg.getLocation(), location)); | ||
assertEquals(TestValues.MATCH, TestValues.GENERAL_DOOR_STATUS_TYPE, status); | ||
} |
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.
@KostyaBoss Below line 37 I would add this line assertEquals(TestValues.MATCH, TestValues.GENERAL_WINDOW_STATE, windowState);
@@ -1,215 +1,423 @@ | |||
/* | |||
* Copyright (c) 2017 - 2019, SmartDeviceLink Consortium, Inc. |
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.
@KostyaBoss It looks like you used the code generator tool to re-generate the entire file. Can you just merge in the changes that are specific to this PR (in the HTML these would be the parameters from driverDoorAjar
to roofStatuses
)?
@@ -202,6 +202,9 @@ public void testJson() { | |||
bodyInformationObj.put(BodyInformation.KEY_IGNITION_STABLE_STATUS, VehicleDataHelper.BODY_INFORMATION_IGNITION_STATUS); |
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.
@KostyaBoss I think you can revert these changes, as vehicle data does not need to be changed for this PR. The corresponding PR for the JavaScript suite (https://github.com/smartdevicelink/sdl_javascript_suite/pull/354/files) did not make changes to the vehicle data.
@@ -320,6 +320,9 @@ public void testJson() { | |||
bodyInformationObj.put(BodyInformation.KEY_PARK_BRAKE_ACTIVE, VehicleDataHelper.BODY_INFORMATION_PARK_BRAKE); | |||
bodyInformationObj.put(BodyInformation.KEY_IGNITION_STABLE_STATUS, VehicleDataHelper.BODY_INFORMATION_IGNITION_STATUS); | |||
bodyInformationObj.put(BodyInformation.KEY_IGNITION_STATUS, VehicleDataHelper.BODY_INFORMATION_IGNITION_STABLE_STATUS); | |||
bodyInformationObj.put(BodyInformation.KEY_ROOF_STATUSES, VehicleDataHelper.ROOF_STATUES); |
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.
@KostyaBoss I think you can revert these changes, as vehicle data does not need to be changed for this PR. The corresponding PR for the JavaScript suite (https://github.com/smartdevicelink/sdl_javascript_suite/pull/354/files) did not make changes to the vehicle data.
@@ -5,16 +5,19 @@ | |||
import com.smartdevicelink.proxy.rpc.BodyInformation; |
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.
@KostyaBoss I think you can revert these changes, as vehicle data does not need to be changed for this PR. The corresponding PR for the JavaScript suite (https://github.com/smartdevicelink/sdl_javascript_suite/pull/354/files) did not make changes to the vehicle data.
@santhanamk I've processed the changes, please re-review the PR |
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.
@KostyaBoss I have gone through the changes. Code looks good. I have given a few more suggestions. Please review whenever you get the chance.
@@ -34,7 +32,7 @@ public void testRpcValues() { | |||
|
|||
// Valid Tests | |||
assertTrue(Validator.validateGrid(msg.getLocation(), location)); |
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.
@KostyaBoss You can delete line 34: assertTrue(Validator.validateGrid(msg.getLocation(), location));
.
@@ -34,7 +32,7 @@ public void testRpcValues() { | |||
|
|||
// Valid Tests | |||
assertTrue(Validator.validateGrid(msg.getLocation(), location)); | |||
assertEquals(TestValues.MATCH, TestValues.GENERAL_DOOR_STATUS_TYPE, status); | |||
assertEquals(TestValues.MATCH, TestValues.GENERAL_GRID, location); |
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.
@KostyaBoss Below line 35 you can bring back the line:
assertEquals(TestValues.MATCH, TestValues.GENERAL_DOOR_STATUS_TYPE, status);
@@ -5,16 +5,19 @@ | |||
import com.smartdevicelink.proxy.rpc.BodyInformation; | |||
import com.smartdevicelink.proxy.rpc.ClusterModeStatus; | |||
import com.smartdevicelink.proxy.rpc.DeviceStatus; | |||
import com.smartdevicelink.proxy.rpc.DoorStatus; |
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.
@KostyaBoss You can revert the changes in VehicleDataHelper.java
.
You can delete the unused import on line 8.
import com.smartdevicelink.proxy.rpc.ECallInfo; | ||
import com.smartdevicelink.proxy.rpc.EmergencyEvent; | ||
import com.smartdevicelink.proxy.rpc.FuelRange; | ||
import com.smartdevicelink.proxy.rpc.GPSData; | ||
import com.smartdevicelink.proxy.rpc.GateStatus; |
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.
@KostyaBoss You can delete the unused import on line 13.
import com.smartdevicelink.proxy.rpc.GearStatus; | ||
import com.smartdevicelink.proxy.rpc.GetVehicleDataResponse; | ||
import com.smartdevicelink.proxy.rpc.Grid; | ||
import com.smartdevicelink.proxy.rpc.HeadLampStatus; | ||
import com.smartdevicelink.proxy.rpc.MyKey; | ||
import com.smartdevicelink.proxy.rpc.OnVehicleData; | ||
import com.smartdevicelink.proxy.rpc.RoofStatus; |
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.
@KostyaBoss You can delete the unused import on line 20.
@@ -48,6 +51,7 @@ | |||
|
|||
import org.json.JSONArray; | |||
import org.json.JSONException; | |||
import org.junit.Test; |
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.
@KostyaBoss You can delete the unused import on line 54.
@@ -147,6 +151,10 @@ | |||
public static final Boolean BODY_INFORMATION_REAR_LEFT_AJAR = false; | |||
public static final Boolean BODY_INFORMATION_REAR_RIGHT_AJAR = true; | |||
|
|||
public static final JSONArray ROOF_STATUES = TestValues.JSON_ROOF_STATUSES; |
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.
@KostyaBoss You can delete lines 154-156.
@santhanamk I've updated the RP, please re-review |
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.
@KostyaBoss Code looks good. I have suggested one more change. Please take a look whenever you get the chance.
DoorStatusType status = msg.getStatus(); | ||
|
||
// Valid Tests | ||
assertEquals(TestValues.MATCH, TestValues.GENERAL_GRID, location); |
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.
@KostyaBoss You can add this line below line 34:
assertEquals(TestValues.MATCH, TestValues.GENERAL_DOOR_STATUS_TYPE, status);
@santhanamk I've processed the changes, please, re-review |
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.
@KostyaBoss Code looks good. I have approved the PR.
Once you have tested against Core and HMI (if applicable), please put the links for those PRs. Please mark the PR as ready for review, and you can tag Livio once the testing is done on your end.
@bilal-alsharifi , @jordynmackool the PR is ready for the review |
base/src/main/java/com/smartdevicelink/proxy/rpc/BodyInformation.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/BodyInformation.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/BodyInformation.java
Outdated
Show resolved
Hide resolved
@bilal-alsharifi I've processed the comments, please, re-review the PR |
Codecov Report
@@ Coverage Diff @@
## develop #1583 +/- ##
=============================================
+ Coverage 53.81% 53.92% +0.11%
- Complexity 4942 4973 +31
=============================================
Files 522 526 +4
Lines 22631 22690 +59
Branches 2823 2823
=============================================
+ Hits 12178 12236 +58
- Misses 9461 9462 +1
Partials 992 992
|
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.
@KostyaBoss finished reviewing and added one more comment.
Fixes #1234
This PR is [ready] for review.
Related PR's
Core
HMI
Risk
This PR makes [minor] API changes.
Testing Plan
Unit Tests
Added unit tests for the new structures
Summary
Changes according to #1234
CLA