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

Fixed: Code style and lint fixes #1261

Merged
coffeedogs merged 2 commits from codeQ-2105 into v0.6 2018-06-12 00:10:53 +02:00
Conversation 0 Commits 2 Files Changed 3 +328 -210
coffeedogs commented 2018-05-24 11:59:25 +02:00 (Migrated from github.com)
Copy Link

Address code style and lint violations in the next two most violating files.

Of interest for testing would be the change to imports. The app runs for me but my testing procedure is not thorough yet.

Address code style and lint violations in the next two most violating files. Of interest for testing would be the change to imports. The app runs for me but my testing procedure is not thorough yet.
g1itch (Migrated from github.com) requested changes 2018-05-24 14:17:29 +02:00
setup.cfg
@ -10,1 +15,3 @@
disable=invalid-name,bare-except
disable=invalid-name,bare-except,broad-except
# invalid-name: needs fixing during a large, project-wide refactor
# bare-except,broad-except: Need fixing once thorough testing is easier
g1itch (Migrated from github.com) commented 2018-05-24 14:06:53 +02:00

So you disable the warnings about except without exception type totally?

So you disable the warnings about except without exception type totally?
src/protocol.py
@ -24,3 +35,4 @@
# Service flags
NODE_NETWORK = 1
NODE_SSL = 2
NODE_DANDELION = 8
g1itch (Migrated from github.com) commented 2018-05-24 14:12:28 +02:00

It seems to be safe to sort these standard library imports. Not sure about the local imports below. Did you test?

It seems to be safe to sort these standard library imports. Not sure about the local imports below. Did you test?
src/protocol.py
g1itch (Migrated from github.com) commented 2018-05-24 13:57:07 +02:00

Maybe this comment should be retained or # Bitfield above deleted?

Maybe this comment should be retained or `# Bitfield` above deleted?
src/protocol.py
g1itch (Migrated from github.com) commented 2018-05-24 14:00:05 +02:00

This should be retained IMHO. It looks like section mark.

This should be retained IMHO. It looks like section mark.
src/protocol.py
@ -253,7 +294,9 @@ def assembleVersionMessage(remoteHost, remotePort, participatingStreams, server
return CreatePacket('version', payload)
g1itch (Migrated from github.com) commented 2018-05-24 13:00:37 +02:00

Again, brackets are redundant here. Also I'd prefer formatting like:

'decryptAndCheckPubkeyPayload failed due to toAddress mismatch.'
' This is very peculiar. toAddress: %s, address %s'

because in this case the continuation line is better seen

Again, brackets are redundant here. Also I'd prefer formatting like: ```python 'decryptAndCheckPubkeyPayload failed due to toAddress mismatch.' ' This is very peculiar. toAddress: %s, address %s' ``` because in this case the continuation line is better seen
g1itch (Migrated from github.com) commented 2018-05-24 14:01:00 +02:00

Next section...

Next section...
src/protocol.py
g1itch (Migrated from github.com) commented 2018-05-24 13:02:42 +02:00

Maybe can you break the line and remove these \

Maybe can you break the line and remove these `\`
src/protocol.py
@ -393,3 +451,4 @@
endOfLifeTime)
return 0
intObjectType, = unpack('>I', data[16:20])
try:
g1itch (Migrated from github.com) commented 2018-05-24 13:05:48 +02:00

Same considerations about brackets and exc_info here and above...

Same considerations about brackets and exc_info here and above...
src/protocol.py
g1itch (Migrated from github.com) commented 2018-05-24 13:04:20 +02:00

Maybe use exc_info=True instead?

Maybe use `exc_info=True` instead?
src/protocol.py
g1itch (Migrated from github.com) commented 2018-05-24 13:15:14 +02:00

Maybe -3600 should be here. That minus is not binary operator.

Maybe `-3600` should be here. That minus is not binary operator.
g1itch (Migrated from github.com) reviewed 2018-05-24 14:24:42 +02:00
src/pyelliptic/arithmetic.py
@ -91,3 +124,1 @@
ripemd160 = hashlib.new('ripemd160')
ripemd160.update(intermed)
return ripemd160.digest()
intermed = hashlib.sha256(string).digest()
g1itch (Migrated from github.com) commented 2018-05-24 14:24:41 +02:00

Strange call. base10_double() is defined for only one arg below.

Strange call. `base10_double()` is defined for only one arg below.
g1itch (Migrated from github.com) reviewed 2018-05-24 14:30:19 +02:00
src/pyelliptic/arithmetic.py
@ -91,3 +124,1 @@
ripemd160 = hashlib.new('ripemd160')
ripemd160.update(intermed)
return ripemd160.digest()
intermed = hashlib.sha256(string).digest()
g1itch (Migrated from github.com) commented 2018-05-24 14:30:19 +02:00

@PeterSurda this looks like unused code

>>> import arithmetic
>>> arithmetic.base10_double(0, 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: base10_double() takes exactly 1 argument (2 given)
>>> arithmetic.base10_add([0, 1], [0, 1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "arithmetic.py", line 53, in base10_add
    if a[1] == b[1]: return base10_double(a[0],a[1])
TypeError: base10_double() takes exactly 1 argument (2 given)
@PeterSurda this looks like unused code ``` >>> import arithmetic >>> arithmetic.base10_double(0, 1) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: base10_double() takes exactly 1 argument (2 given) >>> arithmetic.base10_add([0, 1], [0, 1]) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "arithmetic.py", line 53, in base10_add if a[1] == b[1]: return base10_double(a[0],a[1]) TypeError: base10_double() takes exactly 1 argument (2 given) ```
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:22:51 +02:00
src/protocol.py
coffeedogs (Migrated from github.com) commented 2018-05-24 17:22:51 +02:00

Reluctant to do this as part of code quality / lint work. If you open an issue I'll take a look into this separately.

Reluctant to do this as part of code quality / lint work. If you open an issue I'll take a look into this separately.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:22:55 +02:00
src/protocol.py
coffeedogs (Migrated from github.com) commented 2018-05-24 17:22:54 +02:00

Fix incoming.

Fix incoming.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:22:57 +02:00
src/protocol.py
@ -253,7 +294,9 @@ def assembleVersionMessage(remoteHost, remotePort, participatingStreams, server
return CreatePacket('version', payload)
coffeedogs (Migrated from github.com) commented 2018-05-24 17:22:56 +02:00

OK, change incoming.

OK, change incoming.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:25:56 +02:00
src/protocol.py
@ -393,3 +451,4 @@
endOfLifeTime)
return 0
intObjectType, = unpack('>I', data[16:20])
try:
coffeedogs (Migrated from github.com) commented 2018-05-24 17:25:55 +02:00

Changed to the string formatting incoming, same issue with changing fuctionality here.

Changed to the string formatting incoming, same issue with changing fuctionality here.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:31:35 +02:00
src/protocol.py
coffeedogs (Migrated from github.com) commented 2018-05-24 17:31:34 +02:00

This was already there, in 2.7.13 it seems to do the right thing. Again, raise an issue and I'll check the history of this, perhaps older versions we still support handle it differently so I'm reluctant to change it as part of a style / lint commit.

>>> - 4 < - 3
True
>>> - 2 < - 3
False
This was already there, in 2.7.13 it seems to do the right thing. Again, raise an issue and I'll check the history of this, perhaps older versions we still support handle it differently so I'm reluctant to change it as part of a style / lint commit. ``` >>> - 4 < - 3 True >>> - 2 < - 3 False ```
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:32:24 +02:00
src/protocol.py
coffeedogs (Migrated from github.com) commented 2018-05-24 17:32:24 +02:00

Section markers returning in next commit. I'd prefer separate files for different concerns.

Section markers returning in next commit. I'd prefer separate files for different concerns.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:33:20 +02:00
src/protocol.py
coffeedogs (Migrated from github.com) commented 2018-05-24 17:33:20 +02:00

Section markers returning in next commit

Section markers returning in next commit
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:34:54 +02:00
src/protocol.py
@ -253,7 +294,9 @@ def assembleVersionMessage(remoteHost, remotePort, participatingStreams, server
return CreatePacket('version', payload)
coffeedogs (Migrated from github.com) commented 2018-05-24 17:34:53 +02:00

incoming

incoming
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:38:11 +02:00
setup.cfg
@ -10,1 +15,3 @@
disable=invalid-name,bare-except
disable=invalid-name,bare-except,broad-except
# invalid-name: needs fixing during a large, project-wide refactor
# bare-except,broad-except: Need fixing once thorough testing is easier
coffeedogs (Migrated from github.com) commented 2018-05-24 17:38:11 +02:00

I have explained this issue several times during these cleanup commits. I am unable to fully test which exceptions might be thrown without a more thorough testing setup. If you know for sure which exceptions may be thrown at each bare/broad except then please fill them in and you can take the abuse for any runtime failures that crop up. We know that we have to fix the disabled checks, we're just not doing it now.

I have explained this issue several times during these cleanup commits. I am unable to fully test which exceptions might be thrown without a more thorough testing setup. If you know for sure which exceptions may be thrown at each bare/broad except then please fill them in and you can take the abuse for any runtime failures that crop up. We know that we have to fix the disabled checks, we're just not doing it now.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:41:06 +02:00
src/protocol.py
@ -24,3 +35,4 @@
# Service flags
NODE_NETWORK = 1
NODE_SSL = 2
NODE_DANDELION = 8
coffeedogs (Migrated from github.com) commented 2018-05-24 17:41:06 +02:00

I tested that the gui ran but my test setup is lacking. As I said in the PR description, Of interest for testing would be the change to imports. The app runs for me but my testing procedure is not thorough yet.

I tested that the gui ran but my test setup is lacking. As I said in the PR description, `Of interest for testing would be the change to imports. The app runs for me but my testing procedure is not thorough yet.`
coffeedogs (Migrated from github.com) reviewed 2018-05-24 17:58:59 +02:00
src/pyelliptic/arithmetic.py
@ -91,3 +124,1 @@
ripemd160 = hashlib.new('ripemd160')
ripemd160.update(intermed)
return ripemd160.digest()
intermed = hashlib.sha256(string).digest()
coffeedogs (Migrated from github.com) commented 2018-05-24 17:58:59 +02:00

Try an list of two items:

>>> arithmetic.base10_double([0, 1])
(0L, 115792089237316195423570985008687907853269984665640564039457584007908834671662L)

Although the function is unused (except by arithmetic.base10_add()) there are other functions in there that are not used as well. While removing cruft does fall under linting, I'd argue that applies to our cruft and that we should be using the pyelliptic library rather than copy/pasting it into our code and hacking it.

Perhaps we could open an issue to deal with this separately? Updates to upstream would be easier to manage if we add a dependency or left the code as-is to allow copy/paste from upstream. Having to copy/paste from upstream when we've hacked the files makes updating harder.

If you read this and still want the unused code removing, I will do.

Try an list of two items: ``` >>> arithmetic.base10_double([0, 1]) (0L, 115792089237316195423570985008687907853269984665640564039457584007908834671662L) ``` Although the function is unused (except by arithmetic.base10_add()) there are other functions in there that are not used as well. While removing cruft does fall under linting, I'd argue that applies to our cruft and that we should be using the pyelliptic library rather than copy/pasting it into our code and hacking it. Perhaps we could open an issue to deal with this separately? Updates to upstream would be easier to manage if we add a dependency or left the code as-is to allow copy/paste from upstream. Having to copy/paste from upstream when we've hacked the files makes updating harder. If you read this and still want the unused code removing, I will do.
coffeedogs (Migrated from github.com) reviewed 2018-05-24 18:08:08 +02:00
src/pyelliptic/arithmetic.py
@ -91,3 +124,1 @@
ripemd160 = hashlib.new('ripemd160')
ripemd160.update(intermed)
return ripemd160.digest()
intermed = hashlib.sha256(string).digest()
coffeedogs (Migrated from github.com) commented 2018-05-24 18:08:08 +02:00

It looks like this bug is in upstream since day 1.

It looks like this bug is in upstream since day 1.
g1itch (Migrated from github.com) reviewed 2018-05-24 18:22:44 +02:00
setup.cfg
@ -10,1 +15,3 @@
disable=invalid-name,bare-except
disable=invalid-name,bare-except,broad-except
# invalid-name: needs fixing during a large, project-wide refactor
# bare-except,broad-except: Need fixing once thorough testing is easier
g1itch (Migrated from github.com) commented 2018-05-24 18:22:44 +02:00

I don't request you to add exception types. But I don't like an idea to mask any issue which you cannot resolve immediately. Let lint warn.

I don't request you to add exception types. But I don't like an idea to mask any issue which you cannot resolve immediately. Let lint warn.
g1itch (Migrated from github.com) reviewed 2018-05-24 18:37:25 +02:00
src/protocol.py
@ -24,3 +35,4 @@
# Service flags
NODE_NETWORK = 1
NODE_SSL = 2
NODE_DANDELION = 8
g1itch (Migrated from github.com) commented 2018-05-24 18:37:25 +02:00

OK, at least my tests doesn't fail: https://travis-ci.org/g1itch/PyBitmessage/builds/383283944

OK, at least my tests doesn't fail: https://travis-ci.org/g1itch/PyBitmessage/builds/383283944
g1itch (Migrated from github.com) reviewed 2018-05-24 18:54:15 +02:00
src/protocol.py
g1itch (Migrated from github.com) commented 2018-05-24 18:54:15 +02:00

My remark was exactly about style: I think the space is redundant here. No doubt that works correctly in any python interpreter.

My remark was exactly about style: I think the space is redundant here. No doubt that works correctly in any python interpreter.
coffeedogs (Migrated from github.com) reviewed 2018-05-25 02:30:30 +02:00
setup.cfg
@ -10,1 +15,3 @@
disable=invalid-name,bare-except
disable=invalid-name,bare-except,broad-except
# invalid-name: needs fixing during a large, project-wide refactor
# bare-except,broad-except: Need fixing once thorough testing is easier
coffeedogs (Migrated from github.com) commented 2018-05-25 02:30:30 +02:00

I understand where you're coming from regarding masking warnings. However, there are exactly three lint warnings that are disabled project-wide: invalid-name, bare-except and broad-except. Invalid name will be fixed with a project-wide refactor of names once the other style and lint issues are fixed, the other two require me to have a better test process.

In the meantime we can use the return status of fab code_quality to ensure, as part of our CI and/or git hooks, that we don't regress in terms of code quality. Once we have those lint warnings (that are currently disabled) fixed, the only thing left is the lines that are between 80 and 119 characters, which can be fixed as we go, or when we're slack.

If I were thinking like you are now I would be much more concerned with the three lines worth of lint warnings that I have disabled at the top of the most offending modules. These require serious refactoring in terms of the structure of the module. It cannot be done without a rigorous testing process, I cannot do it personally until I understand the code better. Rest assured, it will be done.

Edit:For example, if we had a 'must not increase violation count' hook and re-enabled the invalid-name check we would never be able to add a line involving a variable namedInCamelCase.

I understand where you're coming from regarding masking warnings. However, there are exactly three lint warnings that are disabled project-wide: invalid-name, bare-except and broad-except. Invalid name will be fixed with a project-wide refactor of names once the other style and lint issues are fixed, the other two require me to have a better test process. In the meantime we can use the return status of `fab code_quality` to ensure, as part of our CI and/or git hooks, that we don't regress in terms of code quality. Once we have those lint warnings (that are currently disabled) fixed, the only thing left is the lines that are between 80 and 119 characters, which can be fixed as we go, or when we're slack. If I were thinking like you are now I would be much more concerned with the three lines worth of lint warnings that I have disabled at the top of the most offending modules. These require serious refactoring in terms of the structure of the module. It cannot be done without a rigorous testing process, I cannot do it personally until I understand the code better. Rest assured, it will be done. Edit:For example, if we had a 'must not increase violation count' hook and re-enabled the invalid-name check we would never be able to add a line involving a variable namedInCamelCase.
coffeedogs (Migrated from github.com) reviewed 2018-05-25 10:18:27 +02:00
src/protocol.py
coffeedogs (Migrated from github.com) commented 2018-05-25 10:18:27 +02:00

No doubt? The linters didn't pick it up. Nor autopep8. So -3600 can't be the canonical version. I'm going assume they know something I don't until I investigate. They have a lot of bugs though, so this could be one of them. Please, raise an issue, assign it to me and I promise I will investigate.

The style I was aiming for here (at Peter's direction) was one that passed codacy but evolved (via github discussion) into one that passes multiple linters.

I don't want to bike-shed about things like hanging brackets in lists/dicts and functions (I like de-dented closes for data and long functions and trailing ones for short functions) or single line #-comments above a line vs a triple-quoted string below (sphinx likes the latter) or the number of statements between single blank lines (I like about 6).

I'm all in favour of us voting on which further, voluntary conventions to stick to. And I will stick to the ones that I don't even like for the sake of the consistency of the project. A consistent project is better than an inconsistent project. But then again, a happy developer is better than an unhappy developer ;)

No doubt? The linters didn't pick it up. Nor autopep8. So `-3600` can't be the canonical version. I'm going assume they know something I don't until I investigate. They have a lot of bugs though, so this could be one of them. Please, raise an issue, assign it to me and I promise I will investigate. The style I was aiming for here (at Peter's direction) was one that passed codacy but evolved (via github discussion) into one that passes multiple linters. I don't want to bike-shed about things like hanging brackets in lists/dicts and functions (I like de-dented closes for data and long functions and trailing ones for short functions) or single line #-comments above a line vs a triple-quoted string below (sphinx likes the latter) or the number of statements between single blank lines (I like about 6). I'm all in favour of us voting on which further, voluntary conventions to stick to. And I will stick to the ones that I don't even like for the sake of the consistency of the project. A consistent project is better than an inconsistent project. But then again, a happy developer is better than an unhappy developer ;)
omkar1117 (Migrated from github.com) approved these changes 2018-05-29 03:57:11 +02:00
PeterSurda (Migrated from github.com) reviewed 2018-06-12 00:08:49 +02:00
src/pyelliptic/arithmetic.py
@ -91,3 +124,1 @@
ripemd160 = hashlib.new('ripemd160')
ripemd160.update(intermed)
return ripemd160.digest()
intermed = hashlib.sha256(string).digest()
PeterSurda (Migrated from github.com) commented 2018-06-12 00:08:49 +02:00

Upstream is abandoned, sadly. We have to carry the flag.

Upstream is abandoned, sadly. We have to carry the flag.
PeterSurda (Migrated from github.com) approved these changes 2018-06-12 00:09:33 +02:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
g1itch
omkar1117
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-08-21#1261
No description provided.
Delete Branch "codeQ-2105"

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?