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.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
