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

OSOE-123: Clean up and simplify OrchardCoreUITestBase #170

Merged
merged 2 commits into from
May 27, 2022

Conversation

0liver
Copy link
Contributor

@0liver 0liver commented May 27, 2022

"S4261:Methods should be named according to their synchronicities",
Justification =
"This is a conversion method, and we want to make it explicit that the result has an async signature.")]
public static MultiSizeTestAsync ToAsync(this MultiSizeTest test) =>
Copy link
Member

Choose a reason for hiding this comment

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

I think it's still misleading becuase you'd assume this method does something similar to Task.Run() instead of just returning Task.CompletedTask. So to me this hurts more than it helps. How about one of these mouthfuls?

  • ReturnCompletedTask
  • WithCompletedTask
  • ToMultiSizeTestAsyncWithCompletedTask

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe .AsCompletedTask()?

Copy link
Member

Choose a reason for hiding this comment

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

That's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that the Completed infix is a lot of implementation noise that shouldn't be there, don't you think? Considering just .AsTask() now.

Copy link
Member

Choose a reason for hiding this comment

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

I think whether a task will actually start a thread or not is useful information, not noise.

"S4261:Methods should be named according to their synchronicities",
Justification =
"This is a conversion method, and we want to make it explicit that the result has an async signature.")]
public static Func<T, Task> ToAsync<T>(this Action<T> action) =>
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@@ -13,49 +13,40 @@ namespace Lombiq.Tests.UI;

public abstract class OrchardCoreUITestBase
{
private const string AppFolder = "AppFolder";
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this isn't using nameof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only that it used to be a method-local constant named appFolder. After moving it to the top of the file it became this - and I didn't catch it.

Lombiq.Tests.UI/OrchardCoreUITestBase.cs Show resolved Hide resolved
@sarahelsaig sarahelsaig merged commit c321416 into dev May 27, 2022
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