Repository navigation
Conversation
Mark the unused #490 inbound-routing surface obsolete and point callers at ProtobufProtocolHandler.Handle(DaqifiOutMessage). The types stay until a future major. Co-authored-by: Tyler Kron <tylerkron@gmail.com>
PR Summary by QodoDeprecate legacy generic inbound protocol routing
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. Avalonia tests stop compiling
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /// wrap frames in this type (issue #490). | ||
| /// </remarks> | ||
| /// <typeparam name="T">The type of the data payload.</typeparam> | ||
| [Obsolete($"Use {nameof(Daqifi.Core.Device.Protocol.ProtobufProtocolHandler)}.{nameof(Daqifi.Core.Device.Protocol.ProtobufProtocolHandler.Handle)}({nameof(DaqifiOutMessage)}) instead. This type will be removed in a future major version.")] |
There was a problem hiding this comment.
2. Non-protobuf callers cannot migrate 🐞 Bug ⚙ Maintainability
The new ObsoleteAttribute messages on GenericInboundMessage<T> and IProtocolHandler name ProtobufProtocolHandler.Handle(DaqifiOutMessage) as their replacement even though those public contracts accept arbitrary payloads and implementations. Consumers using a non-protobuf payload or custom protocol handler therefore receive migration guidance to a method that cannot accept their data or preserve their abstraction.
Agent Prompt
## Issue description
The generic inbound-message type and protocol-handler interface direct every consumer to a protobuf-specific method, although their contracts also support non-protobuf payloads and implementations.
## Fix Focus Areas
- src/Daqifi.Core/Communication/Messages/GenericInboundMessage.cs[7-13]
- src/Daqifi.Core/Device/Protocol/IProtocolHandler.cs[13-25]
- src/Daqifi.Core.Tests/Device/Protocol/ProtobufProtocolHandlerTests.cs[206-235]
## Recommended Fix
Give the generic type and interface members migration messages applicable to their full contracts, such as removal-only guidance when no general replacement exists. Keep the typed `Handle` recommendation on the protobuf-specific implementation methods, and update the reflection test to verify the appropriate message for each API rather than requiring the protobuf replacement for all of them.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// wrap frames in this type (issue #490). | ||
| /// </remarks> | ||
| /// <typeparam name="T">The type of the data payload.</typeparam> | ||
| [Obsolete($"Use {nameof(Daqifi.Core.Device.Protocol.ProtobufProtocolHandler)}.{nameof(Daqifi.Core.Device.Protocol.ProtobufProtocolHandler.Handle)}({nameof(DaqifiOutMessage)}) instead. This type will be removed in a future major version.")] |
There was a problem hiding this comment.
1. Avalonia tests stop compiling 🔗 Cross-repo conflict ≡ Correctness
Marking GenericInboundMessage<T> obsolete emits CS0618 where daqifi-avalonia's NonFiniteStreamedReadingTests still constructs that type. The test project treats every warning as an error, so upgrading it to this Core release causes dotnet test to fail during compilation.
Agent Prompt
## Issue description
The new `Obsolete` attribute on `GenericInboundMessage<T>` produces CS0618 in daqifi-avalonia, whose test project treats warnings as errors. Avoid publishing this deprecation until the consumer no longer constructs the wrapper, or coordinate the consumer migration with this release.
## Fix Focus Areas
- src/Daqifi.Core/Communication/Messages/GenericInboundMessage.cs[7-13]
- Daqifi.Avalonia.Tests/Device/NonFiniteStreamedReadingTests.cs[102-112]
- Daqifi.Avalonia.Tests/Daqifi.Avalonia.Tests.csproj[23-32]
## Recommended Fix
First update daqifi-avalonia's test seam to route the `DaqifiOutMessage` through the typed handler without constructing `GenericInboundMessage<object>`, then publish this obsolete annotation. If coordinated consumer changes cannot land first, defer the annotation rather than requiring a warning suppression for an API the consumer still needs.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
Closing — not worth merging right now. Deprecating public API is a release-planning call, and this one has downstream fallout with little upside:
If you want these gone, it's better to plan the removal for the next major alongside the Avalonia migration. 🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Production routing is
DaqifiDevice→ProtobufProtocolHandler.Handle(DaqifiOutMessage), the path added so streaming frames are not wrapped inGenericInboundMessage<object>(issue #490). The library never calls the older inbound-routing surface. This marks it obsolete and leaves the types in place for a later major.Obsolete, with a message pointing at
ProtobufProtocolHandler.Handle(DaqifiOutMessage):GenericInboundMessage<T>IProtocolHandler.CanHandle/HandleAsyncProtobufProtocolHandler.CanHandle/HandleAsyncHandle(DaqifiOutMessage)is unchanged.IUdpTransport,UdpTransport,CompositeMessageParser, WiFi/PostUsbTransparentModeExitDelay,StopMessagePumps,ContinuousDeviceFinder, andSafeLogare untouched.Public API
Non-breaking. Nothing is removed, so
PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtstay as they are.Microsoft.CodeAnalysis.PublicApiAnalyzersdoes not record[Obsolete](same as theIntelHexParserforwarders: the shipped entries stay, and RS0016/RS0017 stay quiet). Deleting an entry here would be the breaking-change signal, which this PR does not want. A reflection test covers the attribute and the replacement message, because the API files cannot.Tests
ProtobufProtocolHandlertests that only wrapped aDaqifiOutMessageinGenericInboundMessage<object>to callCanHandle/HandleAsyncnow callHandle(DaqifiOutMessage). Facts that existed only for the obsolete surface (CanHandletrue/false,HandleAsyncignoring a non-protobuf payload, the Handle-vs-HandleAsync parity check) are gone.