Skip to content

Fixes bugs in Print with AWS SDK v4#87

Merged
samritchie merged 2 commits intofsprojects:masterfrom
njlr:bugfix/dynamo-utils-v4-nulls
Mar 20, 2026
Merged

Fixes bugs in Print with AWS SDK v4#87
samritchie merged 2 commits intofsprojects:masterfrom
njlr:bugfix/dynamo-utils-v4-nulls

Conversation

@njlr
Copy link
Contributor

@njlr njlr commented Mar 18, 2026

This PR fixes some bugs in AttributeValue.Print that were introduced by the upgrade to AWS SDK v4.
Also fixed is a bug where NS was printing as SN.
Test coverage is improved.

member inline av.IsNSSet = av.NS.Count > 0
member inline av.IsBSSet = av.BS.Count > 0
member inline x.IsNULL = x.NULL.GetValueOrDefault false
member inline av.IsSSSet = notNull av.SS
Copy link
Member

Choose a reason for hiding this comment

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

these three are unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now removed 👍

sprintf "{ N = %A }" (av.B.ToArray())
elif av.SS.Count > 0 then
sprintf "{ B = %A }" (av.B.ToArray())
elif notNull av.SS then
Copy link
Member

@bartelink bartelink Mar 19, 2026

Choose a reason for hiding this comment

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

prefer <> null for these over isNotNull

But actually, I think these should use IsThingSet as a guard in order to correctly handle empty dictionaries when in uninitialized mode, which can still arise if people switch the default config mode to not leave lists/dictionaries null?

Image

Copy link
Contributor Author

@njlr njlr Mar 19, 2026

Choose a reason for hiding this comment

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

I switched over to the Is* properties 👍

@bartelink
Copy link
Member

Thanks for catching these and especially for taking the time to add a test

As alluded to in the comments I suspect there may be some additional straggler = null, <> null, isNull, isNotNull checks that need to become .IsThingSet (many of which are my fault from the original PR, sorry)

@njlr
Copy link
Contributor Author

njlr commented Mar 19, 2026

Thanks for catching these and especially for taking the time to add a test

As alluded to in the comments I suspect there may be some additional straggler = null, <> null, isNull, isNotNull checks that need to become .IsThingSet (many of which are my fault from the original PR, sorry)

I will pick up the remainder in #88 and keep this PR focused on the utils / Print functions if that's OK?

@bartelink
Copy link
Member

I will pick up the remainder in #88 and keep this PR focused on the utils / Print functions if that's OK?

That's fine with me; thanks for all the work (Sam will be doing merging/releasing and a final look-over; I just happen to be watching)

I had a quick scan and I'm not sure there are all that many other occurrences in the end, tho perhaps I didnt look hard enough. One thought was that perhaps the isNull and isNotNull helpers should go (I had thought they were the FSharp.Core ones)

@samritchie samritchie merged commit c93e6c5 into fsprojects:master Mar 20, 2026
4 checks passed
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.

3 participants