Martin Mucha has posted comments on this change. Change subject: core: refactored gathering violations to separate class ......................................................................
Patch Set 48: a) from OO point of view, it's nonsense for validator or TO of validationResuts (ValidationsWithViolatingEntities) to translate anything. This is wrong. Validator / ValidationsWithViolatingEntities should return list of ValidationResults, maybe along with causes (violating entities). b) I don't actually understand how translating of error messages works. I cannot see any _LIST suffixes being resolved to form plural forms of failures or any such translation in property files c) concept of single 'violating' entity for any failure seems really optimistic. d) I cannot grant, that all violations produced by validator has same type. There's too many methods with too complex flow and I'm not that bold to make such statement. —— What I'd suggest is: let validator produce N types of failures with M violation entities, and place translation method where it belongs (command). If architecture does not allow "_LIST" magic, forget about it a produce N messages. We do not have time for changes creating aesthetically pleasing code out of this. It works somehow, move on. That's the rhythm. I'll be super pleased if there's time to fix it after deadline. -- To view, visit https://gerrit.ovirt.org/37154 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If92e3476ad490d5bd5957358ad69be6ace4fd4fa Gerrit-PatchSet: 48 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
