-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Coding best practices: async vs await improvements
#2156
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
Changes from 46 commits
123c6e8
07c024c
cc37929
d5bc104
9b9ec8c
fae4354
877814c
4c06622
f095153
2031123
551d1b7
ebd5dcb
d82f37a
e708509
4f1ddd1
59bd034
2abdbbf
11c1459
9bd1214
59c3551
63fc393
61d5436
056831f
9a6eab8
1cc02f1
cb21556
8343c38
687a250
9629a94
b6c3d82
a38f733
0c1b38b
6896aaf
a9659ed
480ba72
3655677
b11b8ee
a851098
5ea8c89
efd5a31
16e495f
8061814
5c03c96
943d9bb
93331c7
cc4a920
8b10792
6ff1011
5376507
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ public Task<List<Service>> GetAsync() | |
| try | ||
| { | ||
| _logger.LogInformation(() => $"Retrieving new client information for service: {ServiceName}..."); | ||
| _services = _consulServiceDiscoveryProvider.GetAsync().Result; | ||
| _services = _consulServiceDiscoveryProvider.GetAsync().GetAwaiter().GetResult(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are within a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not an issue!.. We will tackle the issue of |
||
| return Task.FromResult(_services); | ||
| } | ||
| finally | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,7 +18,7 @@ private int GetDelay() | |||||||
| { | ||||||||
| var delay = 1000; | ||||||||
|
|
||||||||
| var fileConfig = Task.Run(async () => await _fileConfigurationRepository.Get()).Result; | ||||||||
| var fileConfig = _fileConfigurationRepository.Get().GetAwaiter().GetResult(); | ||||||||
|
||||||||
| int Delay { get; } |
Why not to add async version to the interface?
After that we can make private and public methods as true async →
Task<int> DelayAsync { get; }
| var fileConfig = _fileConfigurationRepository.Get().GetAwaiter().GetResult(); | |
| var fileConfig = await _fileConfigurationRepository.Get(); |
Optional goal is using Task.Delay, if applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expression has been revised to execute synchronously within the synchronous int GetDelay() method.
The proposed suggestion for refactoring is beyond the scope of this pull request. We will refactor the interface in future.
TODO added in commit 6ff1011 ✔️
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, my gosh! 🤯 👇 700 lines of fake changes!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Advice: Copy a clean file from the develop branch in the upstream repository (which requires another copy of the upstream repo), and then apply your 2-3 line change. This is a straightforward manual modification, but it is always effective.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raman-m I was trying to figure out why those "fake changes" even though we have the Note: the right version is Those "fake changes" are because some lines at these files are still as Are you aware of that? Should I undo it even though ? It's important to point out that any person who touch these files using an IDE like VS or VS Code will hit the same problem. Basically if I undo these "fake changes", when someone else touch these files they'll face the same issue. Considering the long way to the repo please consider that we should keep the changes as the Also you can just try to do that on your machine and see/confirm it. Example
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've detailed the root cause of fictitious ("fake") changes here: they are the result of auto-refactoring commands in IDEs such as Visual Studio, Visual Code, Rider, and others.
This issue has persisted since the project's inception in 2016!
Yes, I am! What are you trying to teach me, man? I've been aware of this issue for over a year.
It's irrelevant! The likelihood of encountering this issue remains unchanged. We cannot foresee which operating system the subsequent developer might use. The concern is not regarding future developers, but rather about your current development environment.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in commit a851098 ✔️
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got the same issue using Windows (VS and VS Code), but also using linux. That's because I asked you gently, while I was trying to help. You was totally rough with no reason to act like that. |

Uh oh!
There was an error while loading. Please reload this page.