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

GSP-751: Write Empty File Behavior #751

Merged
merged 11 commits into from
Sep 17, 2021
Merged

Conversation

abyss-w
Copy link
Contributor

@abyss-w abyss-w commented Sep 10, 2021

Write Empty File Behavior

@abyss-w abyss-w changed the title Write Empty File Behavior GSP-751: Write Empty File Behavior Sep 10, 2021
@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #751 (b842377) into master (5df2783) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #751   +/-   ##
=======================================
  Coverage   12.34%   12.34%           
=======================================
  Files          22       22           
  Lines        1434     1434           
=======================================
  Hits          177      177           
  Misses       1250     1250           
  Partials        7        7           
Flag Coverage Δ
unittests 12.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5df2783...b842377. Read the comment docs.

@Xuanwo Xuanwo requested review from a team September 10, 2021 08:18
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
If we want to upload an empty file, we need to do so:

```go
_, err = s3.s.Write(path, bytes.NewReader([]byte{}), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
@abyss-w
Copy link
Contributor Author

abyss-w commented Sep 15, 2021

The above has been received and I will make the changes.

docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
docs/rfcs/751-write-empty-file-behavior.md Outdated Show resolved Hide resolved
}
```

- What will happen if we got a nil `io.Reader` and `size = 0`?
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 writing them like a markdown table?

reader size description
valid zero xxxx
valid valid xxxx
nil zero xxxx
nil valid xxxx

The markdown syntax is:

| reader | size |  description |
| - | - | - |
| valid | zero | xxxx |
| valid | valid | xxxx |
| nil | zero | xxxx |
| nil | valid | xxxx |

If the table is not good for long sentences, how about:

valid reader and valid size

xxx

valid reader and zero size

xxx

nil reader and valid size

xxx

nil reader and zero size

xxx

And at last, we can give a conclusion about all those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got 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.

I think the second way is better.


### nil io.Reader and valid size

- We will return an error. In this case, the user's action is wrong, so we should return an error to alert the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to add a new error code for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the same error that should be covered in https://forum.beyondstorage.io/t/topic/190, how about return error directly and formalize along with next proposal?

@Xuanwo Xuanwo merged commit 5e5d191 into beyondstorage:master Sep 17, 2021
@abyss-w abyss-w deleted the write-behavior branch September 17, 2021 06:02
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.

3 participants