Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

Implement basic operations #13

Merged
merged 12 commits into from
Jul 27, 2021

Conversation

junaire
Copy link
Member

@junaire junaire commented Jul 18, 2021

This is the second PR of my work. It intends to finish tasks below:

  • List
  • Create
  • Delete
  • Metadata
  • Read
  • Stat
  • Write
  • Direr

General tracking issue: #6

@junaire junaire marked this pull request as draft July 21, 2021 12:06
@junaire junaire marked this pull request as ready for review July 22, 2021 06:03
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated
Comment on lines 201 to 210
tmp := strings.Split(path, "/")
for i := 0; i < len(tmp); i++ {
if i == 0 {
fileId, err = s.createDir(ctx, tmp[0], "root")
} else if i != len(tmp)-1 {
fileId, err = s.createDir(ctx, tmp[i], fileId)
} else {
file := &drive.File{Name: tmp[i]}
_, err = s.service.Files.Create(file).Context(ctx).Media(r).Do()
}
Copy link
Member Author

@junaire junaire Jul 26, 2021

Choose a reason for hiding this comment

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

I guess I have misunderstood something. If user calls Write("a/b/c.txt"), but directory a/b is not exist, should we create them for user? If not, these lines could be simply deleted. Ping @Xuanwo to have a look

Copy link
Contributor

Choose a reason for hiding this comment

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

If user calls Write("a/b/c.txt"), but directory a/b is not exist, should we create them for user?

Yes, we need to create dirs for users.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will do a quick fix for this.

@junaire
Copy link
Member Author

junaire commented Jul 26, 2021

I'm really sorry about the messy code but I have to rewrite most of them, and I have no idea why the tests failed. BTW, the cache support will in the next PR.

@Xuanwo Xuanwo mentioned this pull request Jul 26, 2021
@Xuanwo
Copy link
Contributor

Xuanwo commented Jul 27, 2021

I have no idea why the tests failed.

To implement Direr, we need to embed types.UnimplementedDirer in Storage struct.

BTW, the cache support will in the next PR.

LGTM

storage.go Outdated
// pathToId converts path to fileId, as we talked in RFC-14.
// Ref: https://github.com/beyondstorage/go-service-gdrive/blob/master/docs/rfcs/14-gdrive-for-go-storage-design.md
// If success, we will return the fileId of the path we passing, and nil for sure.
// But if the path is not exist, we will return an empty string and error.
Copy link
Contributor

Choose a reason for hiding this comment

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

we will send API in pathToId, how will we handle the API error here?

How about adding a rule that:

  • err represents the error handled in pathToId
  • fileId represents the results: fileId empty means the path is not exist, otherwise it's the fileId of input path.

storage.go Outdated
pathUnits := strings.Split(absPath, "/")
fileId = "root"
// Traverse the whole path, break the loop if we fails at one search
for i := 0; i < len(pathUnits); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using for _, v := range pathUnits here?

storage.go Outdated
fileId = "root"
// Traverse the whole path, break the loop if we fails at one search
for i := 0; i < len(pathUnits); i++ {
fileId, err = s.searchContentInDir(ctx, fileId, pathUnits[i])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check error here, we need to break the for loop ASAP.

@junaire junaire requested a review from Xuanwo July 27, 2021 05:31
storage.go Outdated
@@ -2,35 +2,251 @@ package gdrive

import (
"context"
"fmt"
"github.com/beyondstorage/go-storage/v4/services"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's format the imports.

  • stdlib
  • external libs
  • self libs

storage.go Outdated
)

const DirectoryMimeType = "application/vnd.google-apps.folder"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't export unless we really need it.

@Xuanwo
Copy link
Contributor

Xuanwo commented Jul 27, 2021

Great job!

@Xuanwo Xuanwo merged commit 3b5ca8d into beyondstorage:master Jul 27, 2021
@junaire junaire deleted the implement-basic-operations branch July 27, 2021 05:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants