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

2021-08-25T18:05:51+02:00

kdcis commented

2021-08-25 18:05:51 +02:00

(Migrated from github.com)

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

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

PeterSurda (Migrated from github.com) requested changes 2021-08-26 10:20:36 +02:00

src/shared.py

PeterSurda (Migrated from github.com) commented

2021-08-26 10:19:49 +02:00

text.decode('utf-8')

``` text.decode('utf-8') ```

PeterSurda (Migrated from github.com) commented

2021-08-26 10:20:03 +02:00

except UnicodeDecodeError:

``` except UnicodeDecodeError: ```

g1itch commented

2021-08-26 13:35:02 +02:00

(Migrated from github.com)

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

kdcis (Migrated from github.com) reviewed 2021-08-27 17:06:29 +02:00

src/shared.py

kdcis (Migrated from github.com) commented

2021-08-27 17:06:29 +02:00

Updated

kdcis (Migrated from github.com) reviewed 2021-08-27 17:06:34 +02:00

src/shared.py

kdcis (Migrated from github.com) commented

2021-08-27 17:06:34 +02:00

Updated

PeterSurda (Migrated from github.com) requested changes 2021-08-31 10:29:07 +02:00

PeterSurda (Migrated from github.com) left a comment

Try moving the functions into a separate files.

PeterSurda (Migrated from github.com) reviewed 2021-08-31 10:36:50 +02:00

PeterSurda (Migrated from github.com) left a comment

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

`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 commented

2021-08-31 16:13:38 +02:00

(Migrated from github.com)

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.

> `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.

👍 1

g1itch commented

2021-09-01 16:47:32 +02:00

(Migrated from github.com)

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).

> `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 commented

2021-09-03 10:43:44 +02:00

(Migrated from github.com)

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.

> > `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 commented

2021-09-03 13:21:06 +02:00

(Migrated from github.com)

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):

> 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 commented

2021-09-03 14:01:24 +02:00

(Migrated from github.com)

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

g1itch commented

2021-09-03 14:50:03 +02:00

(Migrated from github.com)

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

the reload hashes is still in the shared

> It's called from reload hashes and this is called from several places. the reload hashes is still in the shared

PeterSurda commented

2021-09-03 15:31:24 +02:00

(Migrated from github.com)

the reload hashes is still in the shared

Yes but I asked for it to be moved too.

> the reload hashes is still in the shared Yes but I asked for it to be moved too.

g1itch commented

2021-09-03 15:53:41 +02:00

(Migrated from github.com)

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.

> > 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 commented

2021-09-04 06:57:25 +02:00

(Migrated from github.com)

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.

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 commented

2021-09-04 20:55:41 +02:00

(Migrated from github.com)

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):

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): ![packages](https://user-images.githubusercontent.com/4012700/132105056-10b6fdce-2849-4531-9c04-4e43ea2bb070.png)

g1itch commented

2021-09-04 21:07:37 +02:00

(Migrated from github.com)

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.

> 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 commented

2021-09-05 06:47:15 +02:00

(Migrated from github.com)

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.

> 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 commented

2021-10-04 14:36:04 +02:00

(Migrated from github.com)

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

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

> 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 (Migrated from github.com) reviewed 2021-10-05 09:04:57 +02:00

src/api.py

PeterSurda (Migrated from github.com) commented

2021-10-05 09:04:56 +02:00

should be in a different file