search

zeromq / netmq

A 100% native C# implementation of ZeroMQ for .NET
Other
2.95k stars 744 forks source link

Annotate internal patterns, utils and some public API #887

Closed drewnoakes closed 4 years ago

drewnoakes commented 4 years ago

Please especially review the few public API changes towards the end.

Note that Assumes.NotNull is [Conditional("DEBUG")] so has no runtime cost in release builds. I try to avoid using ! where possible, and new code should be designed such that Assumes.NotNull isn't needed either. It's not reasonable to change the design on existing code to address all warnings, so adding these debug-only checks seems like a good tradeoff to me.

drewnoakes commented 4 years ago

@somdoron I cannot see why this test is failing. Is it known to be flakey? I see quite a few PRs merged with failing tests.

NetMQ.Tests.XPubSubTests.MultiplePublishers()
  X NetMQ.Tests.XPubSubTests.MultiplePublishers [509ms]
  Error Message:
   Assert.Equal() Failure
                                 ↓ (pos 23)
Expected: ···sage from subscriber
Actual:   ···sage from subscriber 2
                                 ↑ (pos 23)
  Stack Trace:
     at NetMQ.Tests.XPubSubTests.MultiplePublishers() in /home/runner/work/netmq/netmq/src/NetMQ.Tests/XPubSubTests.cs:line 279
somdoron commented 4 years ago

yes, it is flakey...

somdoron commented 4 years ago

LGTM, I think the compiler can figure out some of the assumes itself. You can merge :)