-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Enable VSTHRD200 (Use "Async" suffix for async methods) #4794
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
Conversation
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.
830d006
to
a773496
Compare
@@ -47,7 +47,7 @@ public interface IAsyncSweeper | |||
/// Propose a <see cref="ParameterSet"/>. | |||
/// </summary> | |||
/// <returns>A future <see cref="ParameterSet"/> and its id. Null if unavailable or cancelled.</returns> | |||
Task<ParameterSetWithId> Propose(); | |||
Task<ParameterSetWithId> ProposeAsync(); |
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.
📝 This is the only public API change in this pull request. It's not clear to me if/when this API shipped or whether it's allowed to be renamed at this point. If we can't rename it, I'll revert the change to this file and use a suppression instead.
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.
Microsoft.ML.Sweeper
hasn't shipped in any nuget package, so I think we are safe to rename this.
@harishsk This is now ready for review. Please make specific note of the indicated public API change. |
The changes have been approved. Can you please rebase and merge? |
This rule remains disabled in test code to avoid requiring test methods to be renamed.
This rule relies on a suppression analyzer (#4803) to avoid requiring test methods to be renamed.