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

Jump to bottom

Fixing DOCUMENTS key and raised exception #539

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixing DOCUMENTS key and raised exception #539

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f225a39
Fixing DOCUMENTS key and raised exception
Nibodhika Feb 19, 2020
7fb3efe
Returning the correct path to the folders on Linux
Nibodhika Mar 1, 2020
b887eb5
Removing the FileNotFoundError exception handling
Nibodhika Apr 5, 2020
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
6 changes: 3 additions & 3 deletions plyer/platforms/linux/storagepath.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ def _get_from_user_dirs(self, name):
index = [i for i, v in enumerate(lines)
if v.startswith("XDG_" + name)][0]
return lines[index].split('"')[1]
except KeyError:
return PATHS[name]
except (KeyError, FileNotFoundError):
return self._get_home_dir() + PATHS[name]
Copy link

Choose a reason for hiding this comment

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

Although this change will return the desired path, I don't agree with having default paths and catching the FIleNotFoundError here. PATHS is assuming that by default all those folders are called like that, which is not always correct. I think the developer is the one who should catch the FileNotFoundError exception, and not plyer. For example:

try:
    storagepath.get_documents_dir()
except FileNotFoundError:
    """
    Show a message that the script could not find the folder location. Perhaps
    you can let the user set the Documents directory manually.
    """

Copy link
Author

Choose a reason for hiding this comment

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

In that case I think other changes should be made, the code is already returning a default value if the key is not present in the file, which is prone to the same sort of errors, and that would raise a KeyError exception.

Also while I agree with you that there are good reasons to let the exception propagate, I would like to suggest a few changes:

  1. We should create a unified exception for both cases so that the user should not need to catch multiple exceptions.

  2. If the default folder exists it should be returned instead of raising the exception.

  3. There should be a parameter or a flag to create the default folder if it doesn't exist instead of raising an exception, and this should be the default.

IMO we need to think that this is just one platform of a multi-platform app and users shouldn't be expected to know that a specific platform can fail due to a quirk of how the platform manages folders, nor should they have to create platform specific code outside of plyer.

Copy link

Choose a reason for hiding this comment

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

It's a good idea! For that, I think another pull request should be opened, leaving this one for the DOCUMENTS fix.

Copy link
Author

Choose a reason for hiding this comment

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

Out of curiosity, why do you think two pull requests should be made for this instead of just making the changes in this PR?

Copy link

Choose a reason for hiding this comment

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

This is not a rule, but...

I think the DOCUMENTS fix is ok, so it can be merged instantly. The other part of your pull request is not complete and might change. Although it is not explicit, in here says:

  1. Create a new, appropriately named branch in your local repository for that specific feature or bugfix. (Keeping a new branch per feature makes sure we can easily pull in your changes without pulling any other stuff that is not supposed to be pulled.)

Ideally there should not be a pull request to solve this and that, but one for each bugfix. Here you are presenting at least two different fixes in one plyer facade. Having them in different pull requests is helpful, because as I said, the DOCUMENTS fix is mergeable, and the other changes can be discussed without interfering the first fix.

Copy link
Author

Choose a reason for hiding this comment

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

Just created a new commit to revert the other exception handling (I think they should be merged though, but I didn't wanted to do it to keep the history until this is ready to merge), I'll be working on the way I think linux systems that don't have /.config/user-dirs.dirs should be handled and we can discuss the details in that PR. But I think it should return a default folder instead of failing, especially to keep consistency with other systems where a failure in this method is not expected.

except Exception as e:
raise e

Expand All @@ -44,7 +44,7 @@ def _get_root_dir(self):
return "/"

def _get_documents_dir(self):
directory = self._get_from_user_dirs("DOCUMENT")
directory = self._get_from_user_dirs("DOCUMENTS")
Copy link

Choose a reason for hiding this comment

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

This is right, according to https://cgit.freedesktop.org/xdg/xdg-user-dirs/tree/user-dirs.defaults.

return directory.replace("$HOME", self._get_home_dir())

def _get_downloads_dir(self):
Expand Down