Ori Liel has posted comments on this change. Change subject: restapi: Introducing Scheduling Policy API ......................................................................
Patch Set 3: (2 comments) Looks good, only one minor comment left in the code. Keep in mind that we still need unit-tests for the patch to be complete. Also, have you verified that RSDL generation did not break? http://gerrit.ovirt.org/#/c/28093/3/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/PolicyUnitMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/PolicyUnitMapper.java: Line 21: model.setDescription(entity.getDescription()); Line 22: String policyUnitTypeName = null; Line 23: switch (entity.getPolicyUnitType()) { Line 24: case FILTER: Line 25: policyUnitTypeName = org.ovirt.engine.api.model.PolicyUnitType.FILTER.name().toLowerCase(); You are now doing the right thing, but to make it a bit simpler and more consistent with other mappers in the API, please write two methods: 1) map the API enum to the Engine enum 2) map the Engine enum to the API enum. Our mappers are full of examples of this, if you need a reference. After the methods are written, invoke them here instead of having this code inline. Line 26: break; Line 27: case WEIGHT: Line 28: policyUnitTypeName = org.ovirt.engine.api.model.PolicyUnitType.WEIGHT.name().toLowerCase(); Line 29: break; Line 59: entity.setDescription(model.getDescription()); Line 60: } Line 61: if (model.isSetType()) { Line 62: PolicyUnitType policyUnitType = null; Line 63: switch (model.getType().toUpperCase()) { You are now doing the right thing, but to make it a bit simpler and more consistent with other 'mappers', please have write two methods, one that maps the API enum to the Engine enum, and one which maps the Engine enum to the API enum. Our mappers are full of examples of this, if you need a reference Line 64: case FILTER: Line 65: policyUnitType = PolicyUnitType.FILTER; Line 66: break; Line 67: case WEIGHT: -- To view, visit http://gerrit.ovirt.org/28093 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c419f953a533e94d8be17cd9ba915ccd5a617a6 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Artyom Lukianov <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
