Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String array + Comparer #63

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

JKamsker
Copy link
Contributor

@JKamsker JKamsker commented Oct 10, 2024

Fixes #60

Builds on #62 , so you may want to merge this first

@JKamsker JKamsker changed the title 60 Support StringEqualityAttribute on collections of strings String array + Comparer Oct 10, 2024
@diegofrata
Copy link
Owner

@JKamsker now that first PR is merged, you might want to rebase this one.

@JKamsker
Copy link
Contributor Author

Oh yea... its alot... but it should really be as small as this JKamsker#2

Ill do it asap and let you know :)

You can aswell create a preview version in the meantime of the already merged code in case i am not fast enough 😅

…g-array

; Conflicts:
;	Generator.Equals.DynamicGenerationTests/Base_Assertions.cs
; Conflicts:
;	Generator.Equals.sln
;	Generator.Equals/EqualityGeneratorBase.cs
;	Generator.Equals/EqualityMemberModelTransformer.cs
;	Generator.Equals/SymbolHelpers.cs
@JKamsker
Copy link
Contributor Author

I think its ready @diegofrata

@JKamsker JKamsker mentioned this pull request Oct 20, 2024
@diegofrata
Copy link
Owner

Finally had time to look into it and while I can see the usefulness, I must say I disagree with the approach here.

I believe the user should be able to guess how a type comparison will be done without having to refer to docs -- StringEquality alone does not tell me in which way the comparison works in a collection. Differently from the implicit mode which uses the built-in comparer for each type, I don't think we can pick unordered or ordered as the "correct" default as we don't know what the most common use case is.

I also think combining attributes, while smart, can be problematic. For example, should StringEquality work with UnorderedEquality in a Dictionary? What about SetEquality. Without analyzers, there is no indication to the user to what works together what does not. Particularly if not all cases will be supported out of the box. It also makes the implementation more complicated than needed be.

I think the way forward here is a little less sexy than composing attributes. I see two ways to move this forward:

  1. Have specialised string attributes: OrderedStringEquality, UnorderedStringEquality and SetStringEquality. These should be easy for users to find discover when interacting with package in the IDE.
    OR
  2. The existing equality attributes to carry an optional parameter for StringComparison, that only gets taken into account if generic type argument of the IEnumerable is a string or if the type of the dictionary key is a string.

I am not sure which I prefer. I am leaning towards 1 as it is the most explicit option and also easier to implement.

What do you think?

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.

Support StringEqualityAttribute on collections of strings
2 participants