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

Jobs: .With() extension methods. #289

Merged
merged 4 commits into from
Nov 4, 2016
Merged

Jobs: .With() extension methods. #289

merged 4 commits into from
Nov 4, 2016

Conversation

ig-sinicyn
Copy link
Contributor

As requested by @adamsitnik

Changelist:

  • Jobs: WithXxx() extension methods added back, docs updated.
  • Jobs, minor bug fixed: Id was cleared on explicit JobMode assignment + test for it (Test03IdDoesNotFlow).
  • Jobs.Accuracy, typo: AnaylyzeLaunchVariance => AnalyzeLaunchVariance, docs updated.

Jobs: Fixing Id flow on explicit JobMode assignment + test for it (Test03IdDoesNotFlow)
Jobs.Accuracy: Typo: AnaylyzeLaunchVariance => AnalyzeLaunchVariance, docs updated
@AndreyAkinshin
Copy link
Member

In the original design, the WithXXX() methods don't change the original job, it always creates a copy of the original job with requested change. Could we keep this approach?

@ig-sinicyn
Copy link
Contributor Author

ig-sinicyn commented Oct 21, 2016

@AndreyAkinshin yep, I've think about that, but it feels very counterintuitive.

There're mutable properties, there's .Apply() method that updates the instance of the job and there're .With() methods that create a new copy of the Job on each call. Kinda Js/PHP style : )

I'd prefer to keep standard Create-Init-Freeze-Reuse approach until there're strong reasons against it.

@AndreyAkinshin
Copy link
Member

Ok, let's consider the following situation: I want to have a dry job for Mono. Before the refactoring, I could take the predefined Dry job and create a DryMono job in the following way: Job.Dry.With(Runtime.Mono). It was the main use-case of the WithXXX() methods.

Id flow logic now is the same as with old jobs.
@ig-sinicyn
Copy link
Contributor Author

@AndreyAkinshin

Ok, I've fixed the .With() method logic. Should not break compatibility now.

@ig-sinicyn
Copy link
Contributor Author

ig-sinicyn commented Oct 23, 2016

Oops, last minute fix had broken smth. Fixed


namespace BenchmarkDotNet.Jobs
{
public static class JobExtensions

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@AndreyAkinshin AndreyAkinshin merged commit 307b725 into dotnet:master Nov 4, 2016
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