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

list: fix --trashed to return all items in the current working directory #700

Merged
merged 1 commit into from Jul 29, 2016

Conversation

Projects
None yet
2 participants
@njbbaer
Copy link

commented Jul 26, 2016

Fixes #695.

@odeke-em

This comment has been minimized.

Copy link
Owner

commented Jul 28, 2016

Apologies for the late reply, I'll comment more on this PR in about 9 hours when I get back home.

@@ -142,11 +142,7 @@ func (r *Remote) changes(startChangeId int64) (chan *drive.Change, error) {
func buildExpression(parentId string, typeMask int, inTrash bool) string {
var exprBuilder []string

if inTrash || (typeMask&InTrash) != 0 {

This comment has been minimized.

Copy link
@odeke-em

odeke-em Jul 28, 2016

Owner

Aha I see, so this simplifies the trashed condition checks. Does it work alright with list as well as --trashed in nested directories, not just root?

This comment has been minimized.

Copy link
@njbbaer

njbbaer Jul 28, 2016

Author

Yes, list and list --trashed now work in both root and nested directories. Please test this yourself.

@njbbaer

This comment has been minimized.

Copy link
Author

commented Jul 28, 2016

What broke list --trashed was two separate small bugs. First, it wasn't working in nested directories because it was looking for the directory in trash rather than just its containing files in trash. So I switched list to use its default path resolver FindByPath instead of FindByPathTrashed.

Second, list --trashed returned all trashed items in all directories. I found in buildExpression that a parent directory was not specified if searching inTrash. Adding it back to match the regular case fixed the issue. I also removed the (typeMask&InTrash) != 0 check because I felt it shouldn't contradict the inTrash parameter and simplified the expression even more.

@odeke-em

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

Dope! Thanks @njbbaer. Just one comment here, would you mind updating your commit message to reflect the change with something like this

list: fix --trashed to return all items in the current working directory

Fixes #695

<Any description of the solution if you'd like to make one>
list: fix --trashed to return all items in the current working directory
Fixes #695

1. List trashed uses path resolver FindByPath instead of FindByPathTrashed.
2. Expressions by buildExpression always include parent path.

@njbbaer njbbaer changed the title Fix list -trashed list: fix --trashed to return all items in the current working directory Jul 29, 2016

@njbbaer

This comment has been minimized.

Copy link
Author

commented Jul 29, 2016

Thanks for the tip. I've updated my commit message.

@odeke-em

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

Oh, my bad, I meant in the actual git commit like this

$ git commit --amend

and then when it opens up the editor

list: fix --trashed to return all items in the current working directory

Fixes #695

<Any description of the solution if you'd like to make one>

and after force push it to overwrite this current commit.

$ git push origin <branch_name> -f
@njbbaer

This comment has been minimized.

Copy link
Author

commented Jul 29, 2016

That's what I did. I changed both the commit message and the pull request title.

@odeke-em

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

Awesome. Thank you. LGTM! Merging in.

@odeke-em odeke-em merged commit acca18c into odeke-em:master Jul 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.