Bitmessage/PyBitmessage-2024-12-10
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

Fixed: Style and lint violations for ten more of the worst violating files #1265

Closed
coffeedogs wants to merge 1 commits from codeq31-05 into v0.6
pull from: codeq31-05
merge into: Bitmessage:v0.6
Conversation 2 Commits 1 Files Changed 11 +1734 -899
coffeedogs commented 2018-06-04 12:15:32 +02:00 (Migrated from github.com)
Copy Link

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.

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.
PeterSurda (Migrated from github.com) reviewed 2018-06-04 12:15:32 +02:00
omkar1117 (Migrated from github.com) reviewed 2018-06-04 12:15:32 +02:00
g1itch (Migrated from github.com) reviewed 2018-06-08 15:18:52 +02:00
g1itch (Migrated from github.com) left a comment

Most of this change are destructive I think. Did you tested anything?

Most of this change are destructive I think. Did you tested anything?
src/api.py
@ -11,3 +15,4 @@
import hashlib
import json
from struct import pack
import time
g1itch (Migrated from github.com) commented 2018-06-08 14:59:13 +02:00

Why?

Why?
src/api.py
g1itch (Migrated from github.com) commented 2018-06-08 15:00:38 +02:00

Any function without return statement returns None by default

Any function without `return` statement returns `None` by default
src/api.py
g1itch (Migrated from github.com) commented 2018-06-08 15:03:47 +02:00

It seems you've changed the logic here.

It seems you've changed the logic here.
src/bitmessagemain.py
@ -12,3 +15,2 @@
import os
import sys
The software version variable is now held in shared.py
g1itch (Migrated from github.com) commented 2018-06-08 15:08:14 +02:00

Why are you moving copyrights to docstring?

Why are you moving copyrights to docstring?
src/bitmessagemain.py
@ -30,0 +24,4 @@
import os
import signal
import socket
import sys
g1itch (Migrated from github.com) commented 2018-06-08 15:06:33 +02:00

This will probably break script execution.

This will probably break script execution.
src/bitmessageqt/bitmessageui.py
@ -26,2 +35,3 @@
_encoding = QtGui.QApplication.UnicodeUTF8
def _translate(context, text, disambig, encoding = QtCore.QCoreApplication.CodecForTr, n = None):
def _translate(context, text, disambig, encoding=QtCore.QCoreApplication.CodecForTr, n=None):
g1itch (Migrated from github.com) commented 2018-06-08 14:48:03 +02:00

This changes break portable usage.

This changes break portable usage.
src/bitmessageqt/newaddresswizard.py
@ -1,12 +1,26 @@
#!/usr/bin/env python2.7
g1itch (Migrated from github.com) commented 2018-06-08 14:49:32 +02:00

This is unused module

This is unused module
src/bitmessageqt/settings.py
@ -1,15 +1,23 @@
# -*- coding: utf-8 -*-
g1itch (Migrated from github.com) commented 2018-06-08 14:51:55 +02:00

It's pointless to edit auto-generated module.

It's pointless to edit auto-generated module.
src/network/bmproto.py
g1itch (Migrated from github.com) commented 2018-06-08 14:54:30 +02:00

Docstring imitation.

Docstring imitation.
src/network/tcp.py
@ -148,4 +161,4 @@
elemCount = len(filtered)
if elemCount > maxAddrCount / 2:
elemCount = int(maxAddrCount / 2)
addrs[stream * 2 + 1] = helper_random.randomsample(filtered.items(), elemCount)
g1itch (Migrated from github.com) commented 2018-06-08 14:55:44 +02:00

Oh my eyes

Oh my eyes
src/network/tcp.py
g1itch (Migrated from github.com) commented 2018-06-08 15:15:56 +02:00

You've done the opposite in other hunk.

You've done the opposite in other hunk.
src/proofofwork.py
g1itch (Migrated from github.com) commented 2018-06-08 14:57:27 +02:00

I've vomited

I've vomited
src/upnp.py
g1itch (Migrated from github.com) commented 2018-06-08 15:18:01 +02:00

If result is not used, do not assign anything.

If result is not used, do not assign anything.
coffeedogs commented 2018-06-09 11:07:05 +02:00 (Migrated from github.com)
Copy Link

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.

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.
coffeedogs commented 2018-06-13 20:00:59 +02:00 (Migrated from github.com)
Copy Link

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.

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.
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:02:20 +02:00
src/bitmessageqt/bitmessageui.py
@ -26,2 +35,3 @@
_encoding = QtGui.QApplication.UnicodeUTF8
def _translate(context, text, disambig, encoding = QtCore.QCoreApplication.CodecForTr, n = None):
def _translate(context, text, disambig, encoding=QtCore.QCoreApplication.CodecForTr, n=None):
coffeedogs (Migrated from github.com) commented 2018-06-13 20:02:19 +02:00

Continued in PR#1271

Continued in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:02:36 +02:00
src/bitmessageqt/newaddresswizard.py
@ -1,12 +1,26 @@
#!/usr/bin/env python2.7
coffeedogs (Migrated from github.com) commented 2018-06-13 20:02:36 +02:00

Removed in PR#1271

Removed in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:02:49 +02:00
src/bitmessageqt/settings.py
@ -1,15 +1,23 @@
# -*- coding: utf-8 -*-
coffeedogs (Migrated from github.com) commented 2018-06-13 20:02:48 +02:00

Continued in PR#1271

Continued in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:03:30 +02:00
src/network/bmproto.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:03:30 +02:00

Continued in PR#1271

Continued in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:03:43 +02:00
src/network/tcp.py
@ -148,4 +161,4 @@
elemCount = len(filtered)
if elemCount > maxAddrCount / 2:
elemCount = int(maxAddrCount / 2)
addrs[stream * 2 + 1] = helper_random.randomsample(filtered.items(), elemCount)
coffeedogs (Migrated from github.com) commented 2018-06-13 20:03:43 +02:00

Fixed in PR#1271

Fixed in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:04:51 +02:00
src/proofofwork.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:04:51 +02:00

Continued in PR#1271

Continued in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:05:52 +02:00
src/api.py
@ -11,3 +15,4 @@
import hashlib
import json
from struct import pack
import time
coffeedogs (Migrated from github.com) commented 2018-06-13 20:05:51 +02:00

In the wrong order: Continued in PR#1271

In the wrong order: Continued in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:06:57 +02:00
src/api.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:06:57 +02:00

I know this. Pylint want it explicit here.

I know this. Pylint want it explicit here.
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:07:02 +02:00
src/api.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:07:02 +02:00

Fixed in PR#1271

Fixed in PR#1271
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:21:53 +02:00
src/bitmessagemain.py
@ -12,3 +15,2 @@
import os
import sys
The software version variable is now held in shared.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:21:53 +02:00

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.

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.
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:22:52 +02:00
src/upnp.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:22:52 +02:00

LHS removed - fixed in PR#1271.

LHS removed - fixed in PR#1271.
coffeedogs (Migrated from github.com) reviewed 2018-06-13 20:33:35 +02:00
src/network/tcp.py
coffeedogs (Migrated from github.com) commented 2018-06-13 20:33:35 +02:00

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.

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.
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
PeterSurda
omkar1117
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-12-10#1265
No description provided.
Delete Branch "codeq31-05"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?