Martin Mucha has posted comments on this change. Change subject: core: IdParameters should contain @NotNull validation ......................................................................
Patch Set 13: (1 comment) http://gerrit.ovirt.org/#/c/30569/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java: > This class should override getValidationGroups with "RemoveEntity" group.. a) why to override it there's no reason for that? Or is there one? b) If it should be overridden, why's addValidationGroup protected? If we look at it's usages, we'll find insanities like this: @Override protected List<Class<?>> getValidationGroups() { addValidationGroup(UpdateEntity.class); return super.getValidationGroups(); } which is definitely not a way how it should be used, since we're not altering base method, we're creating side effect(and it's definitely required to add multiple instances of UpdateEntity.class when called more than once). So I'd like to be consistent here: if there's proper base class method, it should be ok to use it. If it's not ok to use it, then it should be not accessible. All ill-designed code, like posted one, should be cured. (This is structurally wrong, and argumentation that this way we improve LOC is improper and also wrong) Line 1: package org.ovirt.engine.core.bll; Line 2: Line 3: import java.util.Collections; Line 4: import java.util.List; -- To view, visit http://gerrit.ovirt.org/30569 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4cda909ffc16aa3b844faa7c2e7cd323a0769db1 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
