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

Fixing DOCUMENTS key and raised exception #539

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

Conversation

Nibodhika
Copy link

Hello, I was trying to use plyer and I found that I couldn't access document folder on my Linux, this was because of two reasons:

 with open(self._get_home_dir() + USER_DIRS, "r") as f:

raises a FileNotFoundError if ~/.config/user-dirs.dirs doesn't exist (but the current code is only handling the case of KeyError, which happens if the key does not exist in user-dirs).

Also line 47 was sending DOCUMENT which worked for the user-dirs.dirs because it started with the same characters, but failed when trying to access PATHS because the key is DOCUMENTS

@Nibodhika
Copy link
Author

Also, I didn't wanted to change that because it might be a reason, but shouldn't the line 33 be:

return self._get_home_dir() + PATHS[name]

Otherwise if the first check fails the function returns /Documents instead of ~/Documents

@Nibodhika
Copy link
Author

Since no one is replying with any counter-argument, I went ahead and fixed it in case this is merged.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for the changes, I left some reviews.

@@ -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.

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.

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.

1 participant