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

Use FileOptions.Asynchronous when doing async IO #2488

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Use FileOptions.Asynchronous when doing async IO #2488

merged 3 commits into from
Jul 5, 2023

Conversation

Neme12
Copy link
Contributor

@Neme12 Neme12 commented Jun 30, 2023 •
edited
Loading

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #2487

@CLAassistant
Copy link

CLAassistant commented Jun 30, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

JimBobSquarePants
JimBobSquarePants requested changes Jul 5, 2023
View reviewed changes
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Looks great, just some naming changes to implement to match project convention. Thanks for helping out!

src/ImageSharp/IO/IFileSystem.cs Show resolved Hide resolved
src/ImageSharp/IO/IFileSystem.cs Show resolved Hide resolved
src/ImageSharp/IO/IFileSystem.cs
/// </summary>
/// <param name="path">Path to the file to open.</param>
/// <returns>A stream representing the file to open.</returns>
/// <returns>A stream representing the opened file.</returns>
Stream OpenReadAsynchronous(string path);
Copy link
Member

Choose a reason for hiding this comment

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

I would use Async as the suffix to match convention.

Copy link
Contributor Author

@Neme12 Neme12 Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@JimBobSquarePants I agree that the Asynchronous suffix looks weird, but I didn't use the Async suffix because the method isn't actually async and doesn't return a Task or anything awaitable. Having that suffix would be misleading and would actually go against the convention. Do you really want to go against the guidelines and have an Async suffix even though the method is synchronous and can't be awaited?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, I misread the code before. Naming is fine then. I'm not that precious about it since it's internal.

@JimBobSquarePants JimBobSquarePants added this to the v3.1.0 milestone Jul 5, 2023
@JimBobSquarePants JimBobSquarePants merged commit c68be04 into SixLabors:main Jul 5, 2023
@Neme12 Neme12 deleted the asyncIO branch July 5, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimBobSquarePants JimBobSquarePants JimBobSquarePants approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

Async IO methods should use FileOptions.Asynchronous
3 participants
@Neme12 @CLAassistant @JimBobSquarePants