Fix creation logic for MSUI_CONFIG_PATH #2343
Conversation
| if not os.path.exists(_dir): | ||
| os.makedirs(_dir) | ||
| # Make sure that MSUI_CONFIG_PATH exists | ||
| _ = fs.open_fs(MSUI_CONFIG_PATH, create=True) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
At current code we have that "unused variable _ " rather seldom. So it is not the first place. So we can keep that one too.
Fixes #2274.
Simpler alternative proposal to #2341.
This works under the assumption that this block of code is just intended to make sure that MSUI_CONFIG_PATH exists (which is the only logical thing I could make out of it).
The distinction between when the variable contains
://and when it does not (which is flawed anyway, since a normal path is allowed to contain that) is not necessary, becausefs.open_fscan just handle both (it treats the case without<scheme>://asosfs://).