Fixed: Style and lint violations for ten more of the worst violating files #1265
Conversation
PeterSurda
added
formatting
g1itch
left a comment
g1itch
left a comment
There was a problem hiding this comment.
Most of this change are destructive I think. Did you tested anything?
| from .messagecompose import MessageCompose | ||
| from .networkstatus import NetworkStatus | ||
| from .blacklist import Blacklist | ||
| from .foldertree import AddressBookCompleter |
There was a problem hiding this comment.
This changes break portable usage.
There was a problem hiding this comment.
Continued in PR#1271
| @@ -1,12 +1,26 @@ | |||
| #!/usr/bin/env python2.7 | |||
There was a problem hiding this comment.
Removed in PR#1271
| @@ -1,16 +1,24 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
It's pointless to edit auto-generated module.
There was a problem hiding this comment.
Continued in PR#1271
| addresses = self._decode_addr() | ||
| for i in addresses: | ||
| seenTime, stream, services, ip, port = i | ||
| """TBC""" |
There was a problem hiding this comment.
Continued in PR#1271
| ) + 2, | ||
| 20 | ||
| ) | ||
| ) * (0.2 + invQueue.queueCount / 2.0) |
There was a problem hiding this comment.
Fixed in PR#1271
| if not network.stats.connectedHostsList(): | ||
| networkStatus = 'notConnected' | ||
| elif len(network.stats.connectedHostsList()) > 0 \ | ||
| elif not network.stats.connectedHostsList() \ |
There was a problem hiding this comment.
It seems you've changed the logic here.
There was a problem hiding this comment.
Fixed in PR#1271
|
|
||
| app_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| os.chdir(app_dir) | ||
| sys.path.insert(0, app_dir) |
There was a problem hiding this comment.
This will probably break script execution.
| Copyright (c) 2012-2016 Jonathan Warren | ||
| Copyright (c) 2012-2018 The Bitmessage developers | ||
| Distributed under the MIT/X11 software license. See the accompanying | ||
| file COPYING or http://www.opensource.org/licenses/mit-license.php. |
There was a problem hiding this comment.
Why are you moving copyrights to docstring?
There was a problem hiding this comment.
Multi-line comments are better done with a triple quoted string. In particular, Sphinx (or rather effective rST comments) requires triple quoted strings. However a triple-quoted string is flagged by linters as 'string statement has no effect'. Perhaps a Sphinx section on copyright would be the best place to put these notices? Being part of a module docstring made more sense to me than loose in the top level in module, though I don't feel a Python file is the right place anyway. So continued in PR#1270.
| # sent out in a big inv message to our new peer. | ||
| for hash, storedValue in bigInvList.items(): | ||
| payload += hash | ||
| for obj_hash, _ in bigInvList.items(): |
There was a problem hiding this comment.
You've done the opposite in other hunk.
There was a problem hiding this comment.
I'm lost, sorry, could you be more specific please? There's one other hunk like that in that file:
- sock, addr = pair
+ sock, _ = pair
But maybe you mean why did I ignore the violation elsewhere, as in # pylint: disable=redefined-outer-name. The answer is because this would change the api of a function. Due to the large number of changes we're making to remove violations we're working on one (or a few) file(s) at a time. There will be some refactorings that will necessarily affect multiple files (e.g. fixing the badly named variables, functions and classes). If you're not referring to either of these, please comment in PR#1271, otherwise, fixed in PR#1271.
| for i in range(50): | ||
| try: | ||
| routerIP, = unpack('>I', socket.inet_aton(router.address)) | ||
| _, = unpack('>I', socket.inet_aton(router.address)) |
There was a problem hiding this comment.
If result is not used, do not assign anything.
There was a problem hiding this comment.
LHS removed - fixed in PR#1271.
|
Sorry @g1itch, @PeterSurda, this PR isn't ready yet, I should have closed it for now. I accept pretty much all your comments there and will address them as soon as I resume work on this branch. I was trying to see how far travis got without the sys path munging. This led to re-factoring imports in other files and eventually started to touch the files affected by open PRs so I was looking to get all the other PRs in before going any further with this one. |
|
I force-pushed and now don't have the permissions to re-open this PR. I opened PR#1271 to track outstanding comments and this can stay closed. |
3 participants
Continuing the code style and linting work. Some docstrings have been deferred until I can provide quick and accurate descriptions.
There's actually 13 files. I was probably looking at only part of git's output when I wrote that. I'll update it.
Travis isn't working. One of these files must take us into a cascading import failure. Currently chasing it down.