Solution for Issue #125 and reverted change made in commit e90fee5 related to {$M-} #126
Conversation
…mpareValue for tkRecord Added ability to register custom comparison routines which cann be used for all types but primarily designed to be used with records
This was removed in commit e90fee5.
…comparer already exists
Source/Delphi.Mocks.Helpers.pas
Outdated
|
|
||
| type | ||
| //Allow custom comparisons | ||
| TCustomValueComparer = reference to function(const a, b: TValue): Integer; |
There was a problem hiding this comment.
No idea if it's possible, but did you try doing something like
TCustomValueComparer = reference to function<T>(const a, b: T): Integer;
that would allow us to avoid TValue
There was a problem hiding this comment.
I don't think that would be possible as I think you might need to declare it as TCustomValueComparer<T> = reference to function<T>(const a, b: T): Integer; so it couldn't be stored in a common container. Having said that, I am sure there should be a way to do this without the need for TValue?
Source/Delphi.Mocks.Helpers.pas
Outdated
| TCustomValueComparer = reference to function(const a, b: TValue): Integer; | ||
| TCustomValueComparerStore = record | ||
| private | ||
| class var CustomComparers: TDictionary<Pointer, TCustomValueComparer>; |
There was a problem hiding this comment.
could this not be
class var CustomComparers: TDictionary<PTypeInfo, TCustomValueComparer>;
ie more strongly typed.
There was a problem hiding this comment.
Yeah, good point, will update that.
|
Nice work. |
|
Thanks and thank you for providing Delphi Mocks on GitHub; I've been really enjoying jumping in to testing using it with DUnitX. |
|
Let me know what you think about the above changes to the comparer behaviour. This seems like a good way to allow the user to see |
|
Changes look good. Can I ask one more thing before I merge this, add a few unit tests that at least exercise this functionality. |
|
Sure, I’ll try get that done this week and push the changes when ready. |
|
BTW, the way you solved getting rid of TValue was ingenious, I would never have thought to use untyped parameters.. tbh I don't think I have ever used them intentionally before :) |
…n a function with untyped parameters
|
Thanks for the praise but I was not overly comfortable with the untyped parameters. I tend to avoid them as it doesn't really fit in well with the general Delphi philosophy of being as strongly typed as possible. So as you will see from the new changes, I have updated the behaviour to rely entirely on generics. It is much better than the untyped parameters as I found that depending on the underlying type in TValue, you cannot always use the raw data like I had. Will get those tests done this week and hopefully this provides some useful functionality to others! |
|
This PR is looking good, however I suspect it might fail with 10.4 Managed records, as they have a specific Kind tkManagedRecord? Did you look managed records? |
|
@alydouglas did you ever look at this with 10.4 and managed records? If you can that would be great. Also you will need to rebase this off master again due to other changes since this PR. |
|
@vincentparrett sorry that I let this slip last year, unfortunately job priorities shifted and I didn't have the time to spend on it. We are now moving across to 10.4 so I will give this another look and see how managed records affects things. |
Added fix for comparing records using a custom comparer that can be registered for use with the TValueHelper.CompareValue routine. Although I added this with the intention of using it for records, I have added it at a higher level on the assumption that other types (or use cases) could benefit from custom comparisons. I'm thinking this could be good for objects when comparing the contents may be preferable?
I have also reverted a change made in e90fee5 where the logic {$M-} for IWeakReferenceableObject was removed. This change unfortunately borked the use of {$M+} at the project level for me. This has had no adverse effects on my use case but this change was made for a reason.