Updated code quality binary operator, ignored bare except, unicode function warnings changes in shared.py

[kdcis]

Updated binary operator line change
Added ignore comments for bare except & unicode function warnings

[g1itch]

What's the point on editing this module? It should be removed soon and even has corresponding docstring.

[PeterSurda]

Try moving the functions into a separate files.

[PeterSurda]

isAddressInMyAddressBook -> src/helper_addressbook.py
isAddressInMySubscriptionsList -> src/helper_addressbook.py
isAddressInMyAddressBookSubscriptionListOrWhitelist -> src/helper_addressbook.py
decodeWalletImportformat -> src/helper_wallet.py
reloadMyAddressHashes -> src/filesystem.py
reloadBroadcastSendersForWhichImWatching -> src/helper_addressbook.py
fixPotentiallyInvalidUTF8Data -> src/helper_unicode.py
checkSensitiveFilePermissions -> src/filesystem.py
fixSensitiveFilePermissions -> src/filesystem.py

[g1itch]

decodeWalletImportformat -> src/helper_wallet.py

did you look into #1796? I moved it into highlevelcrypto. In general I'd suggest rather removing those helpers than produce more.

[g1itch]

fixPotentiallyInvalidUTF8Data -> src/helper_unicode.py

remove? use b''.decode('utf-8', 'ignore') instead?

checkSensitiveFilePermissions -> src/helper_filesystem.py
fixSensitiveFilePermissions -> src/helper_filesystem.py

helper_startup? let's not smear side effects on the codebase.

Also I'd prefer describing the code using packages and modules, not files (or worse - scripts).

[PeterSurda]

fixPotentiallyInvalidUTF8Data -> src/helper_unicode.py

remove? use b''.decode('utf-8', 'ignore') instead?

Ok agree.

checkSensitiveFilePermissions -> src/helper_filesystem.py
fixSensitiveFilePermissions -> src/helper_filesystem.py

helper_startup? let's not smear side effects on the codebase.

I worry we'll have more circular imports if we use helper_startup. These two functions are called from multiple places and not only during startup. So I propose src/filesystem.py.

Also I'd prefer describing the code using packages and modules, not files (or worse - scripts).

Well, there is one question about the final form, and another one about the iterative steps to get there.

[g1itch]

I worry we'll have more circular imports if we use helper_startup. These two functions are called from multiple places and not only during startup.

Where? I don't see it:

$ grep -r checkSensitiveFilePermissions src
src/shared.py:    keyfileSecure = checkSensitiveFilePermissions(os.path.join(
src/shared.py:def checkSensitiveFilePermissions(filename):
$ grep -r fixSensitiveFilePermissions src
src/shared.py:        fixSensitiveFilePermissions(os.path.join(
src/shared.py:def fixSensitiveFilePermissions(filename, hasEnabledKeys):

[PeterSurda]

It's called from reload hashes and this is called from several places.

[g1itch]

It's called from reload hashes and this is called from several places.

the reload hashes is still in the shared

[PeterSurda]

the reload hashes is still in the shared

Yes but I asked for it to be moved too.

[g1itch]

the reload hashes is still in the shared

Yes but I asked for it to be moved too.

Maybe move them together, into the wallet or keymanager module I guess. It's a part of the architectural proposals I was going to describe, which is surprisingly implemented in hyperbit: an object holding all the keys, which performs signing/verification and encryption/decryption and can be locked/unlocked with some additional data (e.g. password). As I see it, it should live in the ObjectProcessor thread.

[PeterSurda]

Well, I'll try to arrange it so that it's not in conflict with your pending PRs, but I think it's OK if it's changed iteratively and moves several times, as long as it keeps working in the meantime. Having more rounds of refactoring shouldn't be an objection, as long as the individual rounds provide some benefit. The purpose of this round is to unburden src/shared.py and reduce cyclical imports.

Regarding key manager, well, I think that's a more complicated issue, as the crypto operations are also used inside singleWorker.

[g1itch]

It is not about the PR conflicts. I'm OK with conflicting PRs if they really make the code better, not just replace one bad formatting by another bad formatting or try to solve an issue already addressed in the existing PR.

I think that a proper refactoring needs more research and planning because you can move it by the small steps trying to not break anything, but if you don't know where are you going to end up, the result is not gonna be great.

The main source of the circular imports I can see now is the network.connectionpool. Not here. And this particular module needs more patient analysis which is cannot be done by a person, not familiar with the code, because it's extremely hard and crucial for the whole project. Let's draw some schemas and write a roadmap, maybe? I don't think that ripping up the codebase into the small pieces each in separate file is gonna help. That's not how the good python programs are written.

pyreverse src/network (2021-05-30 20:04:58):

[g1itch]

Regarding key manager, well, I think that's a more complicated issue, as the crypto operations are also used inside singleWorker.

It doesn't need the private keys and maybe it shouldn't assemble the packets, just setup the PoW. I'm trying to move all the assemble code into the protocol in #1788.

[PeterSurda]

It is not about the PR conflicts. I'm OK with conflicting PRs if they really make the code better, not just replace one bad formatting by another bad formatting or try to solve an issue already addressed in the existing PR.

I don't necessarily disagree, but there are other problems to consider. For example, there is a backlog of PRs because they are often too exhausting to review or too difficult to test. The hurdles for new developers are too high. In summary, there is also a need to make it easier for others to contribute. I find it difficult to find experts to work on the project, and even if I do then there are often too many disagreements to get things done.

I think that a proper refactoring needs more research and planning because you can move it by the small steps trying to not break anything, but if you don't know where are you going to end up, the result is not gonna be great.

There are still steps that can be done that make it easier for new developers to contribute, or for more work to be doable in parallel. I think that this PR as an example of that.

The main source of the circular imports I can see now is the network.connectionpool. Not here. And this particular module needs more patient analysis which is cannot be done by a person, not familiar with the code, because it's extremely hard and crucial for the whole project. Let's draw some schemas and write a roadmap, maybe?

I tend to agree about network.connectionpool, but regarding this file, there is also the difference between short term and long term.

I don't think that ripping up the codebase into the small pieces each in separate file is gonna help. That's not how the good python programs are written.

It makes it easier for a new developer to understand the code. In first step, src/shared.py would only contain global variables and no functions. In further steps, the variables may be moved into modules, or a data class, or something else, similarly with functions. In other words, I think that separating utility functions from global state is helpful even if they don't end up in their final destination immediately.

I have a limited time to spend on the codebase, and I know you have too. Therefore I try to focus on guiding others incrementally. I tried to have others do big planned refactorings, but it mostly doesn't work, even with moderately skilled developers it takes forever and nothing gets finished. For example #1794 appears to be roughly the limit of big refactoring that is still has some benefit.

[g1itch]

In first step, src/shared.py would only contain global variables and no functions.

There is already the state module, containing the global variables.

[PeterSurda]

should be in a different file

[PeterSurda]

helper_sql

[PeterSurda]

helper_sql

[PeterSurda]

helper_sql

[PeterSurda]

Just formatting, no moving around functions.