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

Add FS store #25

Merged
merged 6 commits into from
Aug 16, 2024
Merged

Add FS store #25

merged 6 commits into from
Aug 16, 2024

Conversation

Kuixz
Copy link
Contributor

@Kuixz Kuixz commented Aug 13, 2024

Refer issue #22

Two questions

  1. Where should i put the file fs_test.go?
  2. Path to value should be fsPath/namespace/key or ./fsPath/namepsace/key? -L 34;51

@Kuixz Kuixz requested a review from cychiuae August 13, 2024 09:53
@Kuixz Kuixz force-pushed the fs branch 3 times, most recently from 67a5d12 to dacfd39 Compare August 13, 2024 10:28
@cychiuae cychiuae self-assigned this Aug 13, 2024
@cychiuae
Copy link
Contributor

Where should i put the file fs_test.go?

In golang, we put the test file in the same directory

Path to value should be fsPath/namespace/key or ./fsPath/namepsace/key? -L 34;51

Your implementation doesn't need to care whether it is an absolute path or a relative path but fyi I expect the fsPath will be an absolute path in the config file

pkg/kv/fs.go Outdated Show resolved Hide resolved
pkg/kv/fs.go Outdated Show resolved Hide resolved
pkg/kv/fs.go Outdated Show resolved Hide resolved
pkg/kv/fs.go Outdated Show resolved Hide resolved
pkg/kv/fs.go Outdated
defer s.lock.Unlock()

path := fmt.Sprintf("%s/%s/%s", s.fsPath, ns, key)
s.Touch(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://pkg.go.dev/os#WriteFile, it will automatically create if necessary. You don't need to touch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing it will consistently throw no such file or directory error if not Touch the path beforehand.
I think it might be related to this SO question about os.Create, excerpt:

... you need to ensure that the base directory exists, because os.Create does not handle it.

pkg/kv/fs.go Outdated
Comment on lines 62 to 68
if file, err := os.OpenFile(fpath, os.O_RDONLY|os.O_CREATE, 0644); err != nil {
return err
} else {
return file.Close()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. If all branches of the if-elseif-else are returning something, usually we don't write the else case because it will create too much indentation and make worse readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Golang people like multiple ifs and check err one by one
file, err := os.OpenFile(...)
if err != nil {
  return err
}

err = file.Close()
if err != nil {
  return err
}

return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So strange, why not just return file.Close() 🤔 I will apply this change to os.WriteFile call in Set also

Copy link
Contributor

@cychiuae cychiuae left a comment

Choose a reason for hiding this comment

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

readme.md has conflicts 🤔

pkg/kv/fs_test.go Show resolved Hide resolved
pkg/kv/fs_test.go Outdated Show resolved Hide resolved
@Kuixz Kuixz force-pushed the fs branch 2 times, most recently from 6ff247a to e9cc950 Compare August 14, 2024 06:56
@Kuixz
Copy link
Contributor Author

Kuixz commented Aug 14, 2024

Will there be squash when merging (turn 6 commits into 1)?

@cychiuae cychiuae merged commit a22dbb1 into oursky:master Aug 16, 2024
3 checks passed
@Kuixz Kuixz deleted the fs branch August 19, 2024 01:58
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.

2 participants