-
Notifications
You must be signed in to change notification settings - Fork 30
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
Make the ICodeSaver.SaveCodeToFile method asynchronous #100
Comments
I think this is a great idea. Here is the PR for it: #101 It will go out in the next release once it is merged 😄 Great suggestion @SeanFarrow !! Please keep the suggestions coming! Side note: I have a bunch of code and fluent syntax builders that I want to push, I will soon! I have not had time yet. Thanks again for the suggestion here! |
I did notice this on it: Let's say I have a directory that is like this Then I pass in this path to It will throw an error that it cannot find the path. This is Microsoft What do you guys think about adding a check in there to ensure that it can write to the file? Or we can be more brutal and do something like this: var fi = new FileInfo(path);
Directory.Create(fi.Directory.FullName);
// ~~~ continue to write the file Thats a bit more invasive, but also will make peoples lives 'easier'. Maybe its better to do a check and throw an exception instead? What do you guys think @SeanFarrow @MilleBo |
I would probably check whether the directory exists, creating the directory if it doesn’t.
From: Jeff Ward ***@***.***>
Sent: 28 November 2022 08:08
To: Testura/Testura.Code ***@***.***>
Cc: Sean Farrow ***@***.***>; Mention ***@***.***>
Subject: Re: [Testura/Testura.Code] Make the ICodeSaver.SaveCodeToFile method asynchronous (Issue #100)
I did notice this on it:
Let's say I have a directory that is like this /myDirectory/ with no files in it.
Then I pass in this path to CodeSaver: /myDirectory/someSubDirectory/myfile.cs
It will throw an error that it cannot find the path. This is Microsoft System.IO that throws the error, not us. Its just the nature of System.IO
What do you guys think about adding a check in there to ensure that it can write to the file? Or we can be more brutal and do something like this:
var fi = new FileInfo(path);
Directory.Create(fi.Directory.FullName);
// ~~~ continue to write the file
Thats a bit more invasive, but also will make peoples lives 'easier'. Maybe its better to do a check and throw an exception instead? What do you guys think @SeanFarrow<https://github.com/SeanFarrow> @MilleBo<https://github.com/MilleBo>
—
Reply to this email directly, view it on GitHub<#100 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AALDK7WLWXLGQWDHVUPSOYTWKRR5BANCNFSM6AAAAAASM57NCE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@SeanFarrow - Done, I added the check on both If you have any other ideas please feel free to share! |
@jeffward01 The only things I might do differently are: Other than that, this looks really good. |
@SeanFarrow - no problem, I just implemented the I agree completely that we should implement async methods wherever possible. My personal preference is to implement both Here is a good write-up about it:
The article above refrences this blog post, which is also a good write up: I plan to sweep and implement I saw in some microsoft documentation, that for public facing SDK's they were handling the public async Task SomeMethodAsync(string someParam, CancellationToken cancellationToken = default)
{
// This is the interesting part imo
cancellationToken.ThrowIfCancellationRequested()
// do work...
} Thanks for the suggestion! when I have some time i'll implement more async methods. if there are any blockers for you, feel free to do the same and we'll get it pulled in! |
I completely agree with the above.
Thanks,
Sean.
From: Jeff Ward ***@***.***>
Sent: 28 November 2022 12:35
To: Testura/Testura.Code ***@***.***>
Cc: Sean Farrow ***@***.***>; Mention ***@***.***>
Subject: Re: [Testura/Testura.Code] Make the ICodeSaver.SaveCodeToFile method asynchronous (Issue #100)
@SeanFarrow<https://github.com/SeanFarrow> - no problem, I just implemented the SaveCodeToFileAsync as async because that was the one you had asked for.
I agree completely that we should implement async methods wherever possible. My personal preference is to implement both Sync and Async versions, because in my experience sometimes calling .GetAwaiter().GetResult(); is not the same as a Sync method.
Here is a good write-up about it:
* https://gsferreira.com/archive/2020/08/avoid-getawaiter-getresult-at-all-cost/
* Basically its saying, dont use .GetAwaiter().GetResult();, use Sync instead.
The article above refrences this blog post, which is also a good write up:
* https://odetocode.com/blogs/scott/archive/2019/03/04/await-the-async-letdown.aspx
…________________________________
I plan to sweep and implement async wherever possible, and always include the cancellationToken.
I saw in some microsoft documentation, that for public facing SDK's they were handling the cancellationToken like this:
public async Task SomeMethodAsync(string someParam, CancellationToken cancellationToken = default)
{
// This is the interesting part imo
cancellationToken.ThrowIfCancellationRequested()
// do work...
}
Thanks for the suggestion! when I have some time i'll implement more async methods. if there are any blockers for you, feel free to do the same and we'll get it pulled in!
—
Reply to this email directly, view it on GitHub<#100 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AALDK7VHTPPGCDYXWBDDYF3WKSRFXANCNFSM6AAAAAASM57NCE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
it would be really nice if the ICodeSave.SaveCodeToFile method was made asynchronous. Or maybe we should add a new method and obsolete the old one?
The text was updated successfully, but these errors were encountered: