openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

Some comments inline.

I don't agree with suppressing flake8's warnings, either inline or in a config file, especially the warnings you've suppressed. With our poor test coverage at the moment, we'll find it hard to catch errors due to these mistakes until they end up in church on a Sunday morning. Not pretty.

Diff comments:

> 
> === modified file 'openlp/core/lib/projector/pjlink1.py'
> --- openlp/core/lib/projector/pjlink1.py	2017-06-17 00:25:06 +0000
> +++ openlp/core/lib/projector/pjlink1.py	2017-06-29 03:25:32 +0000
> @@ -44,6 +44,8 @@
>  
>  __all__ = ['PJLink']
>  
> +import re
> +

Both codecs and re are Python core library, why the open line between them?

>  from codecs import decode
>  
>  from PyQt5 import QtCore, QtNetwork
> @@ -331,7 +378,7 @@
>                  self.change_status(E_SOCKET_TIMEOUT)
>                  return
>              read = self.readLine(self.max_size)
> -            dontcare = self.readLine(self.max_size)  # Clean out the trailing \r\n
> +            _ = self.readLine(self.max_size)  # Clean out the trailing \r\n

If you omit the assignment completely, does flake8/pylint complain?

>              if read is None:
>                  log.warning('({ip}) read is None - socket error?'.format(ip=self.ip))
>                  return
> @@ -438,9 +485,13 @@
>              self.check_login(data)
>              self.receive_data_signal()
>              return
> +        elif data[0] != PJLINK_PREFIX:
> +            log.debug("({ip}) get_data(): Invalid packet - prefix not equal to '{prefix}'".format(ip=self.ip,
> +                                                                                                  prefix=PJLINK_PREFIX))
> +            return
>          data_split = data.split('=')
>          try:
> -            (prefix, version, cmd, data) = (data_split[0][0], data_split[0][1], data_split[0][2:], data_split[1])
> +            (version, cmd, data) = (data_split[0][1], data_split[0][2:], data_split[1])
>          except ValueError as e:

You don't actually need the brackets.

  version, cmd, data = data_split[0][1], data_split[0][2:], data_split[1]

>              log.warning('({ip}) get_data(): Invalid packet - expected header + command + data'.format(ip=self.ip))
>              log.warning('({ip}) get_data(): Received data: "{data}"'.format(ip=self.ip, data=data_in.strip()))
> 
> === modified file 'openlp/core/lib/projector/upgrade.py'
> --- openlp/core/lib/projector/upgrade.py	2017-06-09 12:12:39 +0000
> +++ openlp/core/lib/projector/upgrade.py	2017-06-29 03:25:32 +0000
> @@ -25,16 +25,18 @@
>  """
>  import logging
>  
> -# Not all imports used at this time, but keep for future upgrades
> -from sqlalchemy import Table, Column, types, inspect
> -from sqlalchemy.exc import NoSuchTableError
> +from sqlalchemy import Table, Column, types
>  from sqlalchemy.sql.expression import null
>  
> -from openlp.core.common.db import drop_columns
>  from openlp.core.lib.db import get_upgrade_op
>  
>  log = logging.getLogger(__name__)
>  
> +# Possible future imports
> +# from sqlalchemy.exc import NoSuchTableError
> +# from sqlalchemy import inspect
> +# from openlp.core.common.db import drop_columns
> +

Yeah, I'd prefer if these comments were not here (I've been called out for doing the same, so consistency!)

>  # Initial projector DB was unversioned
>  __version__ = 2
>  
> 
> === modified file 'openlp/core/ui/projector/manager.py'
> --- openlp/core/ui/projector/manager.py	2017-06-10 05:57:00 +0000
> +++ openlp/core/ui/projector/manager.py	2017-06-29 03:25:32 +0000
> @@ -527,7 +527,7 @@
>          self.projector_list = new_list
>          list_item = self.projector_list_widget.takeItem(self.projector_list_widget.currentRow())
>          list_item = None
> -        deleted = self.projectordb.delete_projector(projector.db_item)
> +        _ = self.projectordb.delete_projector(projector.db_item)

Again, is it possible to just omit the assignment completely?

>          for item in self.projector_list:
>              log.debug('New projector list - item: {ip} {name}'.format(ip=item.link.ip, name=item.link.name))
>  
> 
> === modified file 'setup.cfg'
> --- setup.cfg	2017-02-26 21:14:49 +0000
> +++ setup.cfg	2017-06-29 03:25:32 +0000
> @@ -1,4 +1,14 @@
> +# E402 module level import not at top of file
> +# E722 do not use bare except, specify exception instead

I would prefer if we always saw these. It's not a good idea to just catch all exceptions, things tend to go wrong silently that way, and our poor users get caught in the middle.

> +# F841 local variable '<variable>' is assigned to but never used

An extremely useful warning, I would prefer seeing it than having my code fail in production.

> +
>  [pep8]
>  exclude=resources.py,vlc.py
>  max-line-length = 120
>  ignore = E402,E722
> +
> +[flake8]
> +exclude=resources.py,vlc.py
> +max-line-length = 120
> +ignore = E402,E722

See my comments above.

> +


-- 
https://code.launchpad.net/~alisonken1/openlp/pjlink2-f/+merge/326493
Your team OpenLP Core is subscribed to branch lp:openlp.