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

Changes based on style and lint checks. (final_code_quality_4) #1362

Closed
coffeedogs wants to merge 1 commits from final_code_quality_4 into v0.6
pull from: final_code_quality_4
merge into: Bitmessage:v0.6
Conversation 4 Commits 1 Files Changed 4 +475 -597
coffeedogs commented 2018-10-10 12:13:14 +02:00 (Migrated from github.com)
Copy Link

Changes based on style and lint checks. (final_code_quality_4)

Changes based on style and lint checks. (final_code_quality_4)
coffeedogs commented 2018-10-20 14:55:03 +02:00 (Migrated from github.com)
Copy Link

Looks like pycodestyle check fails if the file doesn't exist. Not sure where the check is defined but I'm guessing a quick shell check before might work (if the shell is bash: if [ -e $filename ] ; then pycodestyle $filename; fi)

Looks like pycodestyle check fails if the file doesn't exist. Not sure where the check is defined but I'm guessing a quick shell check before might work (if the shell is bash: `if [ -e $filename ] ; then pycodestyle $filename; fi`)
PeterSurda commented 2018-10-20 16:30:15 +02:00 (Migrated from github.com)
Copy Link

@coffeedogs Ok, will check. It's all written in python BTW, no shell. The algorithm for calculating new annotations did have a bug triggerd by removing a file, I thought I fixed it or maybe there's something that I missed.

@coffeedogs Ok, will check. It's all written in python BTW, no shell. The algorithm for calculating new annotations did have a bug triggerd by removing a file, I thought I fixed it or maybe there's something that I missed.
PeterSurda commented 2018-10-27 07:40:38 +02:00 (Migrated from github.com)
Copy Link

Updated pylint checks to ignore E902 (IOError), requeued check.

Updated pylint checks to ignore E902 (IOError), requeued check.
PeterSurda commented 2018-10-27 09:35:35 +02:00 (Migrated from github.com)
Copy Link

Improved handling of removed files.

Improved handling of removed files.
PeterSurda (Migrated from github.com) reviewed 2018-10-27 13:21:29 +02:00
src/class_sqlThread.py
@ -22,0 +32,4 @@
anywhere. Current versions do have support for threading and multiprocessing. I don't see an urgent reason to
refactor this, but it should be noted in the comment that the problem is mostly not valid. Sadly, last time I
checked, there is no reliable way to check whether the library is or isn't thread-safe.
"""
PeterSurda (Migrated from github.com) commented 2018-10-27 13:21:29 +02:00

This actually only applies for certain deployments, and/or really old version of sqlite. I haven't actually seen it anywhere. Current versions do have support for threading and multiprocessing. I don't see an urgent reason to refactor this, but it should be noted in the comment that the problem is mostly not valid. Sadly, last time I checked, there is no reliable way to check whether the library is or isn't thread-safe.

This actually only applies for certain deployments, and/or really old version of sqlite. I haven't actually seen it anywhere. Current versions do have support for threading and multiprocessing. I don't see an urgent reason to refactor this, but it should be noted in the comment that the problem is mostly not valid. Sadly, last time I checked, there is no reliable way to check whether the library is or isn't thread-safe.
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:20:27 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:20:27 +02:00 
        """Incoming object, process it"""
```suggestion """Incoming object, process it""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:28:49 +02:00
src/network/bmproto.py
@ -371,1 +409,4 @@
buffer(self.payload[objectOffset:]), self.object.expiresTime,
buffer(self.object.tag)
)
self.handleReceivedObject(self.object.streamNumber, self.object.inventoryHash)
PeterSurda (Migrated from github.com) commented 2018-10-27 17:28:49 +02:00 
        """Incoming address, process it"""
```suggestion """Incoming address, process it""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:32:59 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:32:59 +02:00 
        """Incoming verack. If already sent my own verack, handshake is complete (except potentially waiting for buffers to flush), so we can continue to the main connection phase. If not sent verack yet, continue processing."""
```suggestion """Incoming verack. If already sent my own verack, handshake is complete (except potentially waiting for buffers to flush), so we can continue to the main connection phase. If not sent verack yet, continue processing.""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:33:16 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:33:16 +02:00 
        """Incoming ping, respond to it."""
```suggestion """Incoming ping, respond to it.""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:33:50 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:33:50 +02:00 
        """Incoming port check request, queue it."""
```suggestion """Incoming port check request, queue it.""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:37:18 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:37:18 +02:00 
        """Incoming request for object(s), if we have them and some other conditions are fulfilled, append them to the write queue."""

PS. the wire protocol works very similarly to Bitcoin, so even if the command isn't documented in Bitmessage Wiki, just look at how it works in Bitcoin.

```suggestion """Incoming request for object(s), if we have them and some other conditions are fulfilled, append them to the write queue.""" ``` PS. the wire protocol works very similarly to Bitcoin, so even if the command isn't documented in Bitmessage Wiki, just look at how it works in Bitcoin.
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:37:59 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:37:59 +02:00

I don' think this is a todo. It's just an explanation of what the ifis doing.

I don' think this is a todo. It's just an explanation of what the `if`is doing.
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:38:37 +02:00
src/network/bmproto.py
@ -522,2 +581,4 @@
logger.debug("Closed connection to %s because I'm connected to myself.",
str(self.destination))
return False
PeterSurda (Migrated from github.com) commented 2018-10-27 17:38:37 +02:00

This however IS a todo. This loop may take a while, during which time a lot of other stuff is locked. Should be refactored in the future.

This however IS a todo. This loop may take a while, during which time a lot of other stuff is locked. Should be refactored in the future.
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:44:19 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:44:19 +02:00 
        """Incoming pong, ignore it. PyBitmessage pings connections after about 5 minutes of inactivity, and leaves it to the TCP stack to handle actual timeouts. So there is no need to do anything when a pong arrives."""
```suggestion """Incoming pong, ignore it. PyBitmessage pings connections after about 5 minutes of inactivity, and leaves it to the TCP stack to handle actual timeouts. So there is no need to do anything when a pong arrives.""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:47:40 +02:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-10-27 17:47:40 +02:00 
        """Incoming version. Parse and log, remember important things, like streams, bitfields, etc."""
```suggestion """Incoming version. Parse and log, remember important things, like streams, bitfields, etc.""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:54:25 +02:00
src/network/bmproto.py
@ -429,6 +480,8 @@ class BMProto(AdvancedDispatcher, ObjectTracker):
return True
def bm_command_version(self):
PeterSurda (Migrated from github.com) commented 2018-10-27 17:54:25 +02:00

I think this isn't necessary. Returning here will prevent verack from being sent. Without that the handshake is considered incomplete, other commands are ignored and the connection will timeout after 20 seconds. The error message in the write buffer has this time to make it through. Probably a leftover from early stages, please remove this "todo".

I think this isn't necessary. Returning here will prevent verack from being sent. Without that the handshake is considered incomplete, other commands are ignored and the connection will timeout after 20 seconds. The error message in the write buffer has this time to make it through. Probably a leftover from early stages, please remove this "todo".
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:55:49 +02:00
src/network/bmproto.py
@ -453,1 +505,4 @@
network.connectionpool.BMConnectionPool().streams,
True,
nodeid=self.nodeid))
if ((self.services & protocol.NODE_SSL == protocol.NODE_SSL) and
PeterSurda (Migrated from github.com) commented 2018-10-27 17:55:49 +02:00 
        """Check the validity of the peer"""
```suggestion """Check the validity of the peer""" ```
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:57:22 +02:00
src/network/bmproto.py
@ -453,1 +505,4 @@
network.connectionpool.BMConnectionPool().streams,
True,
nodeid=self.nodeid))
if ((self.services & protocol.NODE_SSL == protocol.NODE_SSL) and
PeterSurda (Migrated from github.com) commented 2018-10-27 17:57:22 +02:00

Typo?

Typo?
PeterSurda (Migrated from github.com) reviewed 2018-10-27 17:59:50 +02:00
src/socks/__init__.py
@ -1,4 +1,9 @@
"""SocksiPy - Python SOCKS module.
# pylint: disable=too-many-arguments,global-statement,too-many-branches
"""
PeterSurda (Migrated from github.com) commented 2018-10-27 17:59:49 +02:00

This file will be removed once asyncore Socks resolver is finished, so I don't care about it. For P2P connections over Socks we're already using my port of this to asyncore. The only thing missing is a callback mechansim and tests, the actual resolving over wire protocol is implemented, but I never tested it (or maybe I tested it very shortly).

This file will be removed once asyncore Socks resolver is finished, so I don't care about it. For P2P connections over Socks we're already using my port of this to asyncore. The only thing missing is a callback mechansim and tests, the actual resolving over wire protocol is implemented, but I never tested it (or maybe I tested it very shortly).
PeterSurda (Migrated from github.com) reviewed 2018-10-27 18:02:27 +02:00
PeterSurda (Migrated from github.com) left a comment

Change the docstrings according to comments, check the typos. Otherwise OK.

Change the docstrings according to comments, check the typos. Otherwise OK.
coffeedogs (Migrated from github.com) reviewed 2018-10-31 14:11:01 +01:00
src/network/bmproto.py
@ -522,2 +581,4 @@
logger.debug("Closed connection to %s because I'm connected to myself.",
str(self.destination))
return False
coffeedogs (Migrated from github.com) commented 2018-10-31 14:11:00 +01:00

BTW, the rST syntax so that the sphinx documentation can both format and collect a list of all todo items.

BTW, the rST syntax so that the sphinx documentation can both format and collect a list of all todo items.
PeterSurda (Migrated from github.com) reviewed 2018-11-01 07:42:47 +01:00
src/network/bmproto.py
PeterSurda (Migrated from github.com) commented 2018-11-01 07:42:47 +01:00

It doesn't indicate a header presence, it expects it, according to the protocol specification.

It doesn't indicate a header presence, it expects it, according to the protocol specification.
PeterSurda (Migrated from github.com) reviewed 2018-11-01 07:43:10 +01:00
src/network/bmproto.py
@ -176,4 +209,4 @@
if char == "v":
return self.decode_payload_varint()
if char == "i":
return self.decode_payload_node()
PeterSurda (Migrated from github.com) commented 2018-11-01 07:43:10 +01:00

Again, doesn't indicate, but expects, the presence of a command.

Again, doesn't indicate, but expects, the presence of a command.
PeterSurda (Migrated from github.com) requested changes 2018-11-01 07:43:28 +01:00
PeterSurda (Migrated from github.com) left a comment

Minor docstring updates.

Minor docstring updates.
g1itch (Migrated from github.com) reviewed 2019-01-22 10:45:15 +01:00
src/class_sqlThread.py
g1itch (Migrated from github.com) commented 2019-01-22 10:45:15 +01:00

%s causes str() itself - no need to convert args.
use %r instead of str(repr(parameters))

`%s` causes `str()` itself - no need to convert args. use `%r` instead of str(repr(parameters))
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
PeterSurda
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-11-28#1362
No description provided.
Delete Branch "final_code_quality_4"

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?