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.

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: exception improved #2341

Closed
wants to merge 1 commit into from
Closed

Conversation

ReimarBauer
Copy link
Member

Purpose of PR?:

Fixes #2274

Does this PR introduce a breaking change?

If the changes in this PR are manually verified, list down the scenarios covered::

behaviour can be checked by

import logging
import fs

def test_fs_path_not_existing():
    test_path = ("ftp://ftp.de.debian.org/debian/.config/msui", "foo://ftp.de.debian.org/debian/.config/msui")
    for fsurl in test_path:
        try:
            _fs = fs.open_fs(fsurl)
            _fs.makedirs(fsurl)
        except fs.errors.CreateFailed:
            logging.error('FS url "%s" create dir nor supported', fsurl)
            assert "_fs" not in locals()

        except fs.opener.errors.UnsupportedProtocol:
            logging.error('FS url "%s" not supported', fsurl)
            assert "_fs" not in locals()

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

@ReimarBauer ReimarBauer requested review from matrss and joernu76 May 6, 2024 14:36
@ReimarBauer ReimarBauer linked an issue May 6, 2024 that may be closed by this pull request
Comment on lines +41 to +42
except fs.errors.CreateFailed:
logging.error('FS url "%s" create dir nor supported', MSUI_CONFIG_PATH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The logging message does not describe the same error case as fs.errors.CreateFailed. The error is described here: https://docs.pyfilesystem.org/en/latest/reference/errors.html#fs.errors.CreateFailed AFAICT it can be emitted when fs.open_fs fails for some reason and is not necessarily related to if the FS supports creating directories.

Comment on lines 39 to 40
_fs = fs.open_fs(MSUI_CONFIG_PATH)
except fs.errors.CreateFailed:
_fs.makedirs(MSUI_CONFIG_PATH)
Copy link
Collaborator

@matrss matrss May 7, 2024

Choose a reason for hiding this comment

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

This would create a directory tree like MSUI_CONFIG_PATH inside of MSUI_CONFIG_PATH, i.e.:

$ tree test-fs
test-fs

0 directories, 0 files
$ python3
Python 3.11.7 | packaged by conda-forge | (main, Dec 23 2023, 14:43:09) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import fs
>>> a = fs.open_fs("osfs://~/Playground/test-fs")
>>> b = a.makedirs("osfs://~/Playground/test-fs")
>>> 
$ tree test-fs
test-fs
`-- osfs:
    `-- ~
        `-- Playground
            `-- test-fs

5 directories, 0 files

I don't think that is what is intended to happen here. Instead, I think the wanted result is that of fs.open_fs(MSUI_CONFIG_PATH, create=True).

And I think the entire try: block the entire if: block can be replaced with just that (i.e. if there is an exception it should crash MSS and not just be logged as an error, since I'd assume there is no reasonable path forward if the config directory cannot be used).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be radically simplified, I'll open a PR to demonstrate.

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.

Out of Scope Variable in Exception Block in constants.py
2 participants