Saggi Mizrahi has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 4: (5 comments) To sum up. You so whatever you want. Just remember that python have a very low LOC count before things become hard to maintain and though I like sticking it to the man as much as the next guy. Best practices are sometimes called best practices for a reason. http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/config.py.in File packaging/services/ovirt-fence-kdump-listener/config.py.in: Line 10: # distributed under the License is distributed on an "AS IS" BASIS, Line 11: # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. Line 12: # See the License for the specific language governing permissions and Line 13: # limitations under the License. Line 14: > you are not aware of devenv environment of engine, this is common for all, I don't like py.in files. You could put it in a conf and read it. Also, I would assume devenv would put this stuff in the env as this is classically what env vars are for. But I'm not really inclined to start this argument here. Leave it as it is. Line 15: Line 16: DEV_PYTHON_DIR = '@DEV_PYTHON_DIR@' Line 17: ENGINE_VARS = '@ENGINE_VARS@' Line 18: http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 16: import psycopg2 Line 17: Line 18: import logbase Line 19: Line 20: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-fence-kdump-listener') > we use _ for the entire engine as C convention for gettext, this is the def Your code your rules. As long as it's local I guess it's fine. Line 21: Line 22: Line 23: @logbase.Logable() Line 24: class Manager(object): http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/logbase.py File packaging/services/ovirt-fence-kdump-listener/logbase.py: Line 16: Line 17: from ovirt_engine import base Line 18: Line 19: Line 20: class Logable(base.Base): > please look at other implementation within ovirt-engine code base. Having a base object to inherit just because is nothing short of ridiculous. Unless there is a reason you don't need it. But again, your code your rules. Line 21: def __call__(self, target): Line 22: _logger = logging.getLogger(self._LOG_PREFIX + self.__module__) Line 23: setattr(target, "_logger", _logger) http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 50: pidfile, Line 51: ): Line 52: # Check the required engine directories and files: Line 53: self.check( Line 54: os.path.join( > on the opposite, throughout the code base of engine we eliminate the tempor Your code your rules. Line 55: self._config.get("ENGINE_USR"), Line 56: "services", Line 57: ), Line 58: directory=True, http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/qconsumer.py File packaging/services/ovirt-fence-kdump-listener/qconsumer.py: Line 77: self._db_manager.open_connection() Line 78: message = self._queue.get() Line 79: if message['status'] is None: Line 80: latest = self._get_latest(message['host']) Line 81: if ( > again, I disagree, this convention is used all over the engine python code The whole point of the abstract 80 line limit is to eliminate excessive code nesting and the reader trying to guess what an expression means. It took me a while to visually parse the condition and to understand what the condition does. Especially as it goes against the recommended python standard of putting the operators at the end of the previous line to denote continuation. But again, your code your rules. Line 82: latest is None Line 83: or ( Line 84: message['received'] > ( Line 85: latest['received'] -- To view, visit http://gerrit.ovirt.org/27201 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieec3bad47bbba860a52a9ff4e2eb7f61277f4e36 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@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
