Skip to content

Type specific equal constraints#4882

Merged
stevenaw merged 5 commits intonunit:mainfrom
manfred-brands:typeSpecificEqualConstraints
Nov 26, 2024
Merged

Type specific equal constraints#4882
stevenaw merged 5 commits intonunit:mainfrom
manfred-brands:typeSpecificEqualConstraints

Conversation

@manfred-brands
Copy link
Member

@manfred-brands manfred-brands commented Nov 10, 2024

Fixes #4874
Fixes #4875
Fixes #4877
Fixes #2492

This is a merge of #4847, #4848, #4853 with one additional commit restricting combining Using with other modifiers.

Once Using is selected, a user supplied comparer will be called which does not have access to any Within or IgnoreCase modifiers.

The classes are now such that once a constraint specific modifiers is selected, the return type will not have the Using modifiers.
The reverse also holds, once the Using modifiers is used, the return type has no other modifiers.

Use extension methods to remove duplication.

Once a user comparer is specfied other modifiers are invalid.
Once constraint modifiers are specified one no longer can use a user comparer.

.IgnoreCase.Using() or .Using().IgnoreCase makes no IgnoreCase
as the user comparer has no access to the IgnoreCase
@stevenaw
Copy link
Member

Thanks @manfred-brands
I won't be able to review tonight, but your notes about Using prompted my memory back to #2492.

I'm not suggesting we account for that here already, but it did occur to me that the type-specialized equalconstraints added here could also make future features of that nature easier too.

@manfred-brands manfred-brands force-pushed the typeSpecificEqualConstraints branch from 46fd3e9 to 2f99d81 Compare November 10, 2024 11:00
@manfred-brands
Copy link
Member Author

your notes about Using prompted my memory back to #2492.

It sometimes seems you know all outstanding issues, or have a good search method.

I'm not suggesting we account for that here already, but it did occur to me that the type-specialized equalconstraints added here could also make future features of that nature easier too.

I added a commit to fix #2492

I also worked on adding an overload to specify StringComparison but that became to big and not sure if it adds anything.

@stevenaw
Copy link
Member

stevenaw commented Nov 10, 2024

Thanks @manfred-brands !
It was not my intention to add to the scope here by mentioning that ticket, but your adding the fix is a great example of how these can improve usability along with type-safety.

I hope to have time to review within a few days

This to prevent the ambiguity that the Comparer matches 4 different overloads.
@manfred-brands manfred-brands force-pushed the typeSpecificEqualConstraints branch from 2f99d81 to 879c489 Compare November 11, 2024 00:05
Copy link
Member

@OsirisTerje OsirisTerje left a comment

Choose a reason for hiding this comment

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

Awesome work! But it is quite a lot. I'll pull it down and try it out on some projects, that is probably the easiest way to check it out.

@OsirisTerje
Copy link
Member

Checked #4874 , #4875, and #4877 : All works :-)

@OsirisTerje
Copy link
Member

Anything else you want to do here, @manfred-brands ? Or can we merge?

@manfred-brands
Copy link
Member Author

Anything else you want to do here, @manfred-brands ? Or can we merge?

Are you happy to merge to main or do we create a vNext?

@OsirisTerje OsirisTerje self-requested a review November 25, 2024 10:43
Copy link
Member

@OsirisTerje OsirisTerje left a comment

Choose a reason for hiding this comment

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

lgtm

@manfred-brands Can we merge this in now?

@manfred-brands
Copy link
Member Author

@OsirisTerje I have no access to a PC this week to resolve any possible conflicts. Feel free to merge otherwise I'll do it next week.

@stevenaw
Copy link
Member

@manfred-brands I see no merge conflicts here at the moment. I'm happy to merge this in now for you so that it can land before other PRs, eliminating the chance of future merge conflicts.

Thanks again for your contribution here to make these APIs faster and more usable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants