Skip to content

Conversation

@accumulator
Copy link
Member

@accumulator accumulator commented Aug 27, 2025

perform filesystem R/W checks at startup. This avoids raising Exceptions later when actual writes are performed (e.g. #4151).

Not sure yet how portable this is, needs testing on win and mac

Copy link

@zrawsig zrawsig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested a refactor to improve the readability, small grammar nitpick and suggested to move the new function to the first half of the file to group all similar functions together and not mix with the classes.

for f in ['config', 'blockchain_headers', 'recent_servers']:
fpath = os.path.join(electrum_path, f)
if os.path.exists(fpath):
if not os.access(fpath, os.R_OK | os.W_OK):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The os.access() check is repeated a few times with the exception message.
This could be factored out to improve readability, for example:

def check_permissions(path: str):
    if not os.access(path, os.R_OK | os.W_OK):
        raise Exception(f'Cannot read/write {path}')

def check_fs_permissions(config: 'SimpleConfig'):
electrum_path = config.electrum_path()
if not os.access(electrum_path, os.R_OK | os.W_OK):
raise Exception('can not read/write root folder at ' + electrum_path)
Copy link

@zrawsig zrawsig Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"can not" is grammatically incorrect, please change to "Cannot". See the factored out example above.

util.trigger_callback('recently_opened_wallets_update')


def check_fs_permissions(config: 'SimpleConfig'):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only function below the classes, while other generic functions such as this one lives in the beginning of the file. Maybe it could be moved there to keep the file organized that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants