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.

Sign up for GitHub

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

Jump to bottom

Implement API AppendAllBytes #93519

Merged
merged 32 commits into from
Oct 24, 2023
Merged

Implement API AppendAllBytes #93519

merged 32 commits into from
Oct 24, 2023

Conversation

MojtabaTajik
Copy link
Contributor

@MojtabaTajik MojtabaTajik commented Oct 14, 2023 •
edited
Loading

Added AppendAllBytes API and the async version with the corresponding tests.

Fix #84532

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 14, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Oct 14, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Move AppendAllBytes tests to a new file
  • Add separate test files for AppendAllBytes and AppendAllBytesAsync
  • Cover more edge scenarios in tests
  • Remove unnecessary delete command for test file because the test file will be removed by parent dir cleanup by test disposal
Author: MojtabaTajik
Assignees: -
Labels:

area-System.IO, new-api-needs-documentation, community-contribution
Milestone: -

This was referenced Oct 14, 2023
Open
Closed
@MojtabaTajik MojtabaTajik changed the title Api append all bytes 84532 API AppendAllBytes 84532 Oct 18, 2023
@MojtabaTajik MojtabaTajik changed the title API AppendAllBytes 84532 Implement API AppendAllBytes 84532 Oct 18, 2023
jozkee
jozkee reviewed Oct 23, 2023
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/File/AppendAllBytes.cs Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/File/AppendAllBytes.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/File/AppendAllBytesAsync.cs Outdated Show resolved Hide resolved
MojtabaTajik and others added 3 commits October 23, 2023 20:33
@MojtabaTajik @jozkee
Remove extra space
f9ced06 
Co-authored-by: David Cantú <[email protected]>
@MojtabaTajik @jozkee
Remove extra space
d84f25a 
Co-authored-by: David Cantú <[email protected]>
@MojtabaTajik @jozkee
update description of API
db52294 
Co-authored-by: David Cantú <[email protected]>
jozkee
jozkee reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Almost there.

When you commit changes, can you please push when you have all changes ready or use the "Add suggestion to batch" in GH; each push may trigger a CI run which is more expensive than having one after all feedback is addressed.

src/libraries/System.IO.FileSystem/tests/File/AppendAllBytes.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/File/AppendAllBytes.cs Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/File/AppendAllBytes.cs Show resolved Hide resolved
src/libraries/System.IO.FileSystem/tests/File/AppendAllBytesAsync.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Show resolved Hide resolved
MojtabaTajik and others added 3 commits October 23, 2023 21:32
@MojtabaTajik @jozkee
Apply suggestions from code review
11ec838 
- Remove unwanted spaces in files
- Fix API documentation
- Add assertation for file exists

Co-authored-by: David Cantú <[email protected]>
@MojtabaTajik
Add test for scenario when directory not exists
4eca487
@MojtabaTajik
Fix Async documentation and match it with none-async version
73ad221
This was referenced Oct 23, 2023
Closed
Closed
Closed
Closed
@jozkee jozkee merged commit f28a8a7 into dotnet:main Oct 24, 2023
172 of 175 checks passed
@MojtabaTajik MojtabaTajik deleted the API_AppendAllBytes-84532 branch October 24, 2023 19:08
@MojtabaTajik MojtabaTajik changed the title Implement API AppendAllBytes 84532 Implement API AppendAllBytes Oct 25, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jozkee jozkee jozkee approved these changes

Assignees
No one assigned
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: File.AppendAllBytes
2 participants
@MojtabaTajik @jozkee