Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix creation logic for MSUI_CONFIG_PATH #2343

Merged
merged 1 commit into from
May 8, 2024
Merged

Fix creation logic for MSUI_CONFIG_PATH #2343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions mslib/msui/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,8 @@
# ToDo refactor to generic functions, keep only constants
HOME = os.path.expanduser(f"~{os.path.sep}")
MSUI_CONFIG_PATH = os.getenv("MSUI_CONFIG_PATH", os.path.join(HOME, ".config", "msui"))
if '://' in MSUI_CONFIG_PATH:
try:
_fs = fs.open_fs(MSUI_CONFIG_PATH)
except fs.errors.CreateFailed:
_fs.makedirs(MSUI_CONFIG_PATH)
except fs.opener.errors.UnsupportedProtocol:
logging.error('FS url "%s" not supported', MSUI_CONFIG_PATH)
else:
_dir = os.path.expanduser(MSUI_CONFIG_PATH)
if not os.path.exists(_dir):
os.makedirs(_dir)
# Make sure that MSUI_CONFIG_PATH exists
_ = fs.open_fs(MSUI_CONFIG_PATH, create=True)
Copy link
Member

@ReimarBauer ReimarBauer May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if we don't need _ we can just use fs.open_fs(MSUI_CONFIG_PATH, create=True)

but I think we should catch unsupported protocols. I bundled a few, but there are some more available, that helps with a typo too.

it will crash now, but maybe we could tell the reason more user friendly

Copy link
Collaborator Author

@matrss matrss May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if we don't need _ we can just use fs.open_fs(MSUI_CONFIG_PATH, create=True)

The _ is a conventional name for an unused variable. I did that intentionally to acknowledge that the method has a return value, but also to show that we are only interested in its side effects. I feel like implicitly unused return values are a code smell, since it is impossible to know then if the omission was intentional or not. This kind of thinking might be a bit odd in Python, it is much more important in other languages where return values are used to communicate errors (which really should be handled, while ignoring can be a way to handle them, if done consciously) like Go, C or Rust.

I don't feel too strongly about this, so I can change it if you want.

it will crash now, but maybe we could tell the reason more user friendly

How do you want it to look for a user?

Copy link
Member

@ReimarBauer ReimarBauer May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are too many possibilities and I think we can not catch it there. That changes my opinion. The logging did not help in that case because we store to MSUI_CONFIG_PATH. So the behaviour of the fix is fine.

Just tried a few

export MSUI_CONFIG_PATH=/boot/

there the path exists, it gets over the line and crashes later.

or

export MSUI_CONFIG_PATH=/home/otheraccount/.config/msui

there it gets in a permission denied and can't create so the cause is clear.

The user needs to do an active action to change the default and when doing that he should understand the problem.

on a fs-server-urls there will be some more,

Maybe we can introduce later a msui --check-config so that a user knows afterwards what to do or can give us that information with a traceback.

Copy link
Member

Choose a reason for hiding this comment

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

At current code we have that "unused variable _ " rather seldom. So it is not the first place. So we can keep that one too.


GRAVATAR_DIR_PATH = fs.path.join(MSUI_CONFIG_PATH, "gravatars")

Expand Down
Loading