Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Dec 22, 2017

No description provided.

@pakrym pakrym requested a review from BrennanConroy December 22, 2017 19:25

namespace Microsoft.AspNetCore.SignalR.Microbenchmarks
{
public class CoreConfig : ManualConfig
Copy link

Choose a reason for hiding this comment

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

Is this configuration no longer required, or did the logic move somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to common: dotnet/extensions#303

@pakrym
Copy link
Contributor Author

pakrym commented Dec 22, 2017

Travis failure is due to unrelated flakiness

@@ -0,0 +1 @@
[assembly: BenchmarkDotNet.Attributes.AspNetCoreBenchmark]
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to change this...i'm just curious. Would does this work if you put it into the csproj file?

<ItemGroup>
   <AssemblyAttribute Include="BenchmarkDotNet.Attributes.AspNetCoreBenchmark" />
</ItemGroup>

If so, you could add this piece of MSBuild into the Benchmark.Sources package so it's automatically applied to any benchmark project that uses it.

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 don't want things that implicit. There might be a case where configs need to be overridden and having source file is much easier than trying to find some packages target that brought attribute and figure out how to change it.

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 could've just dropped assembly info into source package too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants