set file's `trashed` attribute to true instead of permanently removing it #178

robeeeert · 2017-04-11T07:58:25Z

This is a PR for issue #177
I wasn't able to test it though, because I couldn't setup my debugging environment.
Also, it is not configurable by FUSE variables yet.

So, if anybody is interested in this, please feel free to expand on this or provide me with suggestions on how to improve it :)

dsoprea · 2017-04-11T13:18:46Z

Thank you. It was a good suggestion and I was hoping to see a PR.

You can use Vagrant to easily test it (vagrant/).

GDFS needs to maintain its current behavior and permanently delete files unless you turn it on via mount variables.

robeeeert · 2017-04-11T16:45:46Z

Thanks for the vagrant hint. I tested it and found I used a deprecated method of trashing files, so I used the up-to-date function.

Concerning the variable: Would it be okay to use an environment variable like GD_DEBUG or do you prefer a FUSE variable? I'm asking since I don't have any experience with FUSE, so it could take a while until I can apply the changes.

dsoprea · 2017-04-11T17:16:35Z

Just consider it to be a mount parameter. It's not FUSE specific. These are the properties passed via "-o" when you mount any filesystem. This is necessary as probably no variables will be defined when filesystems are mounted at system startup. Add a field to https://github.com/dsoprea/GDriveFS/blob/master/gdrivefs/conf.py#L10 with a default value of `False` and use `conf.Conf.get('delete_to_trash')` from your patch to get the value. Update `set` to coerce the incoming string value ("0", "1") to a bool (`bool(int(value))`) if `key` is equal to "delete_to_trash".

…

On Apr 11, 2017 12:45 PM, "robeeeert" ***@***.***> wrote: Thanks for the vagrant hint. I tested it and found I used a deprecated method of trashing files, so I used the up-to-date function. Concerning the variable: Would it be okay to use an environment variable like GD_DEBUG or do you prefer a FUSE variable? I'm asking since I don't have any experience with FUSE, so it could take a while until I can apply the changes. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#178 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArrarWNCzcQlfMy25w_RdKrqfwiKd0fks5ru646gaJpZM4M5vrM> .

…ing and file deletion

robeeeert · 2017-04-11T20:24:49Z

Yea, makes sense! Thank you for your guidance! :)

One last question, out of curiosity: Why should the default behavior be to delete the files and not to trash them? Is it only backwards-compatibility or is it by design?
I naively would argue it should default to trashing the files, since I don't see any downsides: Locally, the file is gone, but you still have the option to restore it from "the cloud", with "the cloud" acting as a backup system.

dsoprea · 2017-04-11T20:30:39Z

gdrivefs/conf.py

@@ -54,5 +57,7 @@ def get(key):
    def set(key, value):
        if key not in Conf.__dict__:
            raise KeyError(key)
+        elif key is "delete_to_trash":


In get() make sure to use an inequality, not is. is is only used when checking that one instance is the same as another. By contrast, creating two different objects (instances) from the same class should always return False. Note that string caching may create inconsistent results when comparing strings. Consequently, you should only use is for comparing False, True, and None.

dsoprea · 2017-04-11T20:46:19Z

gdrivefs/gdtool/drive.py

@@ -770,10 +770,15 @@ def remove_entry(self, normalized_entry):

        client = self.__auth.get_client()

-        args = { 'fileId': normalized_entry.id }
+        args = {


We should modify the existing function (remove_entry). We should be adding a new function called trash_entry. These hit two different APIs. Just update the existing references from remove_entry to trash_entry. Thanks. We're almost there.

But then I have to check for delete_to_trash before every call to remove_entry. Shouldn't we then have something like a delegate function that either calls remove_entry or trash_entry based on the variable's value?

I'm tempted to agree, but two points I'd like to make:

This module is a general API wrapper. There should be minimal intelligence. We should put branching logic in another module.

There is always the chance that we'll have a need to either definitely trash things or definitely delete things, later. We can't not prevent us from being able to do that.

Please create a "gdutility" module in the same package (path) with a class called "GdUtility" and add a method called "smart_delete" that calls the appropriate method.

dsoprea · 2017-04-11T20:46:50Z

Can you please remove the comment at https://github.com/dsoprea/GDriveFS/blob/master/gdrivefs/gdfs/gdfuse.py#L670?

dsoprea · 2017-04-11T20:50:03Z

It was an oversight. Usually Unix filesystems at the console don't have a trash. On the other hand, maybe others assumed that it went into the trash but never mistakenly deleted anything important. For right now, let's just implement it and have some people test it. Then, we can adjust the default.

If a few people would like to volunteer to check that this works as expected, we can change the default after. Technically this is a break in backwards-compatibility, but since Google will GC the trashed items (and the quota won't be negatively affected for very long except for massive deletes, which is hardly the common use-case in GD) the risk is not very high.

…based on the delete_to_trash config variable

robeeeert · 2017-04-14T16:45:36Z

Hey, just wanted to know if you're still into this thing and if you're content with the proposed approach :)

dsoprea

Sorry for the delay. I never submitted the review.

dsoprea · 2017-04-11T21:39:36Z

gdrivefs/gdtool/drive.py

@@ -770,10 +770,15 @@ def remove_entry(self, normalized_entry):

        client = self.__auth.get_client()

-        args = { 'fileId': normalized_entry.id }
+        args = {


I'm tempted to agree, but two points I'd like to make:

This module is a general API wrapper. There should be minimal intelligence. We should put branching logic in another module.

There is always the chance that we'll have a need to either definitely trash things or definitely delete things, later. We can't not prevent us from being able to do that.

Please create a "gdutility" module in the same package (path) with a class called "GdUtility" and add a method called "smart_delete" that calls the appropriate method.

robeeeert · 2017-04-19T22:57:31Z

Alright, thanks! I'll continue this weekend :)

…rt_delete

robeeeert · 2017-04-22T20:23:50Z

Hi again, I moved the method into its own module. Are you sure we need a class for it? I considered declaring it a static method, since we don't need any state (hardly ever in a utility class generally I guess), and then discovered the "python way" was to simply implement it as a normal function.
What's your opinion on this?

set file's attribute to true instead of permanently removing it

d6a1181

use APIv2 syntax for trashing a file

68e1b3a

introduce delete_to_trash mount variable to switch between file trash…

1d3fb58

…ing and file deletion

dsoprea reviewed Apr 11, 2017

View reviewed changes

fix string comparison by using == instead of is

ed2174d

dsoprea reviewed Apr 11, 2017

View reviewed changes

robeeeert added 2 commits April 11, 2017 22:55

remove TODO comment

28b9697

create delegate method that either calls remove_entry or trash_entry …

a07613a

…based on the delete_to_trash config variable

dsoprea requested changes Apr 16, 2017

View reviewed changes

move delegation method into own module gdutility and rename it to sma…

2b6fc66

…rt_delete

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set file's `trashed` attribute to true instead of permanently removing it #178

set file's `trashed` attribute to true instead of permanently removing it #178

robeeeert commented Apr 11, 2017 •

edited

dsoprea commented Apr 11, 2017

robeeeert commented Apr 11, 2017

dsoprea commented Apr 11, 2017 via email

robeeeert commented Apr 11, 2017

dsoprea Apr 11, 2017

dsoprea Apr 11, 2017

robeeeert Apr 11, 2017

dsoprea Apr 11, 2017

dsoprea commented Apr 11, 2017

dsoprea commented Apr 11, 2017 •

edited

robeeeert commented Apr 14, 2017

dsoprea left a comment

dsoprea Apr 11, 2017

robeeeert commented Apr 19, 2017

robeeeert commented Apr 22, 2017

set file's trashed attribute to true instead of permanently removing it #178

Are you sure you want to change the base?

set file's trashed attribute to true instead of permanently removing it #178

Conversation

robeeeert commented Apr 11, 2017 • edited

dsoprea commented Apr 11, 2017

robeeeert commented Apr 11, 2017

dsoprea commented Apr 11, 2017 via email

robeeeert commented Apr 11, 2017

dsoprea Apr 11, 2017

Choose a reason for hiding this comment

dsoprea Apr 11, 2017

Choose a reason for hiding this comment

robeeeert Apr 11, 2017

Choose a reason for hiding this comment

dsoprea Apr 11, 2017

Choose a reason for hiding this comment

dsoprea commented Apr 11, 2017

dsoprea commented Apr 11, 2017 • edited

robeeeert commented Apr 14, 2017

dsoprea left a comment

Choose a reason for hiding this comment

dsoprea Apr 11, 2017

Choose a reason for hiding this comment

robeeeert commented Apr 19, 2017

robeeeert commented Apr 22, 2017

set file's `trashed` attribute to true instead of permanently removing it #178

set file's `trashed` attribute to true instead of permanently removing it #178

robeeeert commented Apr 11, 2017 •

edited

dsoprea commented Apr 11, 2017 •

edited