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

refactor file cache code #25

Merged
merged 1 commit into from
May 10, 2023

Conversation

rfyiamcool
Copy link
Contributor

@rfyiamcool rfyiamcool commented May 7, 2023

what

  1. add rwlock for file cache, to ensure thread-safe.
  2. refactor file_test.go by assert mode.
  3. mkdirall dir when new file cachego.

@rfyiamcool rfyiamcool changed the title adjust file cache code refactor file cache code May 7, 2023
Copy link
Owner

@faabiosr faabiosr left a comment

Choose a reason for hiding this comment

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

Thanks you so much for you contribution, however I added some points regarding the changes. Also because is such a small change, I recommend you to squash all commits, and follow the commit message instructions. https://github.com/faabiosr/cachego/blob/master/CONTRIBUTING.md#create-a-commit

file/file.go Show resolved Hide resolved
file/file_test.go Outdated Show resolved Hide resolved
@rfyiamcool
Copy link
Contributor Author

Thanks you so much for you contribution, however I added some points regarding the changes. Also because is such a small change, I recommend you to squash all commits, and follow the commit message instructions. https://github.com/faabiosr/cachego/blob/master/CONTRIBUTING.md#create-a-commit

oh, done, i merge multiple commit to one commit.

Copy link
Owner

@faabiosr faabiosr left a comment

Choose a reason for hiding this comment

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

Regarding the last commented about the removed tests. Also I recommend you to rebase this PR with the latest master changes (made by you 😄 ).

file/file_test.go Outdated Show resolved Hide resolved
@faabiosr
Copy link
Owner

@rfyiamcool Did you rebase your branch with the latest changes?

@faabiosr faabiosr merged commit 4d7e104 into faabiosr:master May 10, 2023
3 checks passed
@faabiosr
Copy link
Owner

By the way, thanks for the great contribution @rfyiamcool !

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.

None yet

2 participants