Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
I've never been a fan of these 'scalar or object' types.
Honestly this should be changed to a null object type thing and be given a hierachy.
New type, 2 subtype with implicit converters to their actual type. So consumer can use regular type checks rather than null validation.
I did something similar in other places.
Edit: they already have these types.. why the hell do they have the integers as well.
I'll dig further into the code
bielu
changed the title
|
God I love Claude sometimes |
…olete MQTTServerBinding.SessionExpiryInterval, MaximumPacketSize and MQTTOperationBinding.MessageExpiryInterval are int? views over the MQTTIntegerOrSchema values. They return null when the value is a schema or above Int32.MaxValue, which MQTT allows (up to 4294967295; for the session expiry interval 0xFFFFFFFF means "never expires"). Callers cannot tell that apart from "not set". Keep the properties for compatibility, but document the behaviour and mark them [Obsolete] pointing to the *Value properties. Tests now use the *Value properties. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MQTTIntegerOrSchema and MQTTStringOrSchema are now abstract bases with MQTTInteger/MQTTIntegerSchema and MQTTString/MQTTStringSchema subtypes, so callers use type checks instead of null checks. Implicit conversions from long/string/AsyncApiJsonSchema keep assignments such as SessionExpiryInterval = 600 compiling, and each subtype converts back to its underlying value. The lossy int?/string accessors and the *Value properties are removed; SessionExpiryInterval, MaximumPacketSize, MessageExpiryInterval and ResponseTopic now expose the union type directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
617306c to
ccdb44e
Compare
|
@VisualBean i have love/hate relation with claude, as it makes me faster in delivery of packages, but cognitive load is much higher 🙄 |
And thank god for that. Only means you still care for the result. It makes a lot of things faster, but also makes mistakes like a junior 😅 its all trade-offs 😁. |
|
@VisualBean yeah my approach is to treat it as working with junior developer 😂 |
100% same for me. Extreme programming style + a lot of swearing 😂. |
This branch has not been deployed
2 participants
Problem
Since #45, these MQTT fields can be an integer/string or a schema, but each was exposed twice: a union object with nullable
Integer/LongInteger/Schemamembers, plus a scalar accessor kept for backward compatibility:MQTTServerBinding.SessionExpiryInterval(int?)SessionExpiryIntervalValueMQTTServerBinding.MaximumPacketSize(int?)MaximumPacketSizeValueMQTTOperationBinding.MessageExpiryInterval(int?)MessageExpiryIntervalValueMQTTMessageBinding.ResponseTopic(string)ResponseTopicValueThe
int?accessors returnednullfor schema values and for values aboveInt32.MaxValue. MQTT allows up to 4294967295, and a0xFFFFFFFFsession expiry means "never expires", so it couldn't be told apart from "not set". Consumers also had to null-check members of the union to find out which alternative was present.Change
Following review feedback, the unions are now a type hierarchy (same approach as
AsyncApiAvroSchema):MQTTIntegerOrSchema(abstract) →MQTTInteger(long Value) |MQTTIntegerSchema(AsyncApiJsonSchema Schema)MQTTStringOrSchema(abstract) →MQTTString(string Value) |MQTTStringSchema(AsyncApiJsonSchema Schema)long/string/AsyncApiJsonSchemato the base, and from each subtype back to its underlying value.*Valueproperties are removed. The four properties above now expose the union type directly, asMQTT5ServerBinding.SessionExpiryIntervalalready did.Breaking change
v3.0.1 shipped these four properties as
int?/string. Assignments still compile through the implicit conversions. Code that reads them asint?/stringhas to switch to type checks. TheFromInteger/FromLong/FromSchema/FromStringfactories and theInteger/LongIntegermembers (unreleased, from #45) are replaced by the subtype constructors.The full suite passes on net8.0/net9.0/net10.0.
🤖 Generated with Claude Code