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

refactor: reuse already implemented methods in Namespaces #238

Merged
merged 3 commits into from
Sep 4, 2023
Merged

refactor: reuse already implemented methods in Namespaces #238

merged 3 commits into from
Sep 4, 2023

Conversation

winstxnhdw
Copy link
Contributor

@winstxnhdw winstxnhdw commented Sep 1, 2023

  • Reuse implemented methods on overloads

No idea why I didn't write it like this originally.

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Nice improvement 👍
I would however stick to the async/await pattern even for these simple one-liners. Omitting the await keyword trades valuable features for a minimal performance gain. Since our library is not performance-critical (it is not in anyone's hot loop) it is better to actually await all calls. Primarily, for the stack trace completeness in case of exceptions.

@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Sep 4, 2023

I would however stick to the async/await pattern even for these simple one-liners. Omitting the await keyword trades valuable features for a minimal performance gain. Since our library is not performance-critical (it is not in anyone's hot loop) it is better to actually await all calls. Primarily, for the stack trace completeness in case of exceptions.

I am not very sure how async/await works in C#. My intellisense tells me that an awaitable is propagated whether or not the child functions are async. Doesn't this mean that as long as the parent function is async, all child functions will also be awaitable? I am also following how the other APIs are implemented, like Role and Catalog.

@marcin-krystianc
Copy link
Contributor

I am not very sure how async/await works in C#. My intellisense tells me that an awaitable is propagated whether or not the child functions are async. Doesn't this mean that as long as the parent function is async, all child functions will also be awaitable? I am also following how the other APIs are implemented, like Role and Catalog.

Your code is technically correct, but there is a nice write-up from Stephen Cleary (https://blog.stephencleary.com/2016/12/eliding-async-await.html) explaining all of this. Generally it is better to not elide async/await keywords unless there is an important reason to do it.

@winstxnhdw
Copy link
Contributor Author

Your code is technically correct, but there is a nice write-up from Stephen Cleary (https://blog.stephencleary.com/2016/12/eliding-async-await.html) explaining all of this. Generally it is better to not elide async/await keywords unless there is an important reason to do it.

Thank you for the great article. After reading it, I think we are following the article’s recommended guidelines. The author suggests to consider the following.

Do consider eliding when the method is just a passthrough or overload.

In this case, the methods which are elided are indeed passthrough or overloads. Should I still add the async/await modifiers?

@marcin-krystianc
Copy link
Contributor

Should I still add the async/await modifiers?

I think yes because that makes it easier to analyze stack traces as they are more complete (when the async/await is omitted, such a method will not be included in the stack trace). Maybe in this case it is not a big problem, but in general, gaps in the stack trace can make it much harder to follow.

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

👍

@marcin-krystianc marcin-krystianc merged commit 33ef0bc into G-Research:master Sep 4, 2023
147 checks passed
@winstxnhdw winstxnhdw deleted the refactor/namespace branch September 4, 2023 20:39
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.

2 participants