fn: Added HasDuplicates function to slice
#8750
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
fn/slice.go
Outdated
| @@ -198,3 +198,7 @@ func Sum[B Number](items []B) B { | |||
| return a + b | |||
| }, 0, items) | |||
| } | |||
|
|
|||
| func IsUniqueVals[A comparable](items []A) bool { | |||
There was a problem hiding this comment.
Rename to HasDups and invert the equality check.
"IsUniqueVals" is a bit awkward in English.
Otherwise this looks great
There was a problem hiding this comment.
Also add a godoc for this
There was a problem hiding this comment.
Thanks for the review. I will add the godoc, HasDups also sounds to me like awkward English. I guess there is really no standard for this ? Just subjectiveness?
There was a problem hiding this comment.
Ultimately all naming decisions are subjective. However, just because there is an element of taste involved doesn't mean that there aren't better and worse choices.
There is a standard around the prefixes isX and hasX when a function returns a bool. The question you are attempting to answer in full english is any of the following:
- "Does this slice have any duplicate values?"
- "Are all of the values in this slice unique?"
Since the argument to the function is a singular slice, the slice is the subject of the sentence. The answer to the question then takes the following form.
- "The slice has only unique values"
- "The slice has duplicate values"
Both of these are equivalent statements and, from an awkwardness perspective, are fine. Translating this to function names we might get something like:
hasOnlyUniqueValshasDuplicateValshasDuplicatessince "duplicate" is also a noun in English ("unique" is not)
From there we can use the very common abbreviation of "duplicate" as "dup" to arrive at a quite short and also intuitive name of hasDups.
If you think that "dup" is confusing, we can expand it to hasDuplicates instead. I'm flexible on how aggressively we abbreviate names but insist we use as precise of language as we can in naming things, specifically because these functions become canonical very very quickly. Most FP utility libraries don't actually have this function (I checked) so we can't copy their naming. In fact when doing some research to come up with the name I suggested I stumbled on people discovering that this is not a typical functional name.
Here are some of the findings of that research:
- There are things in the Haskell ecosystem that are close.
- Ramda doesn't have something that checks for uniqueness but it does have a function that computes it.
- Lodash is the same story.
- IBM's fp-go is the same story as well
- Scala doesn't have this operation either but instead of
uniqit's called distinct
So you're right that there is not a standard name for this specific function... mostly because people don't need it very often. I haven't seen how you plan to use it so I can't comment on whether it is needed, however, if we are going to put that function in this package we should name it as best as possible because utility package functions get more use than average and the fact that fn gets version pinned means that changing the names is nontrivial. The standardness that does exist is around both isX and hasX being normal for bool returning functions.
These explanations take me a considerable amount of time to put together. I am giving you this one as a courtesy because this is a subject I both know and care deeply about. In the future, I would really suggest you be more cooperative in code review and put at least as much effort into trying to understand your reviewer's comments as you do contesting them. I do not ask you to rename things just to be difficult. I would appreciate it if you were less stubborn in accepting feedback going forward.
There was a problem hiding this comment.
Thanks for sharing this with me.
I would appreciate it if you were less stubborn in accepting feedback going forward.
Do you mean not asking for clarification, when one does not genuinely follow or agree with the reviewer? I do not think anyone would be growing if they did that. Not everyone is as smart and experienced as some though, we do not all think alike, contributors all come from different background ( is open source any way). What might be obvious to one might not be to the other. So I think reviewers ( and authors) should be open to explaining when need be. It is a different case when the author of a code is being unreasonable stubborn which so far from thread and others, it is interesting to me that, that is anyone's conclusion.
I think writing things, like "awkward", "unreadable" etc. might need to come with justifying explanations from the writer's POV so that the reader can understand their perspective. I think those are vague words and interpretation varies from person to person.
Hence, I think the below should not be and should be a norm in code review sessions?
I am giving you this one as a courtesy because this is a subject I both know and care deeply about.
Interesting take I have seen you, other contributors , reviewers etc. write paragraphs in PRs and issues explaining their POV. I have not seen anyone come to this conclusion that because one disagrees with the other they are not being cooperative.
I would really suggest you be more cooperative in code review and put at least as much effort into trying to understand your reviewer's comments as you do contesting them. I do not ask you to rename things just to be difficult. I would appreciate it if you were less stubborn in accepting feedback going forward.
I see why you think it might be awkward English, according to you the the slice is the subject and it would be awkward to be like, slice is unique values. I was just looking at it from the perspective of the values not really the structure that holds them, so something like, "are unique values", I was not sure I had seen a function that starts with, "are" so I just settled for using "is" at the beginning.
The name can be improved upon, out of your well curated and researched list, I think I would go with hasDuplicates That way I would not have to change the logic as well?
There was a problem hiding this comment.
I see why you think it might be awkward English, according to you the the slice is the subject and it would be awkward to be like, slice is unique values. I was just looking at it from the perspective of the values not really the structure that holds them, so something like, "are unique values", I was not sure I had seen a function that starts with, "are" so I just settled for using "is" at the beginning.
This is a fine intuition, and ultimately a good attempt. I just wasn't satisfied with it as is so I provided an alternative that I believe to be better so that I didn't burden you with having to figure it out on your own.
The name can be improved upon, out of your well curated and researched list, I think I would go with hasDuplicates That way I would not have to change the logic as well?
HasDuplicates is a fine option and I would support its addition. However, you still would need to invert the equality check since the presence of duplicates means that the values are not all unique.
Regarding Cooperation and Review
Do you mean not asking for clarification, when one does not genuinely follow or agree with the reviewer?
You may always ask for clarification, but you must consider the cost of giving that clarification. Wanting clarification is free. Giving clarification is at least to some degree costly. Choose wisely what you ask about. If your choices of how and when to dig in on an issue result in being a net cost to the project, people will deprioritize engaging with you.
So I think reviewers ( and authors) should be open to explaining when need be.
I am open to explaining any and all of my positions as I have done many times with you before. I'm not fundamentally against explaining things. I like doing so. I find myself frustrated by the fact that every single thing I ask for in your reviews that isn't part of a "codified standard" is challenged without an attempt to foster a cooperative spirit.
I have been asked many times in my career to do things that I either didn't completely understand or agree with. In those cases I stated my case, but ultimately communicated to those I was working with/for that I would defer to their judgement and that I would appreciate an explanation if they were willing to do so. We aren't entitled to explanations, so it's best to act in a way that makes people want to give them.
Hence, I think the below should not be and should be a norm in code review sessions?
It's not the norm. It also isn't the norm for someone to DOS the maintainer's time by demanding explanations for small requested changes when the maintainer has already done the work of giving the solution.
I have not seen anyone come to this conclusion that because one disagrees with the other they are not being cooperative.
Cooperative disagreement is nuanced. It is a give and take between hearing points and making them. Saying "I don't agree", "I don't follow", or "Can you explain why?" without demonstrating your own thoughtfulness or reasoning creates asymmetry in the effort put in by the parties involved. I don't mind that you disagree. The fact that you resist changes without offering concessions, demonstrating a genuine desire to understand, or providing mutually satisfying alternatives, makes it very difficult to make progress on these reviews.
I know that maybe it might feel like I'm attacking you with this feedback but I'm actually genuinely trying to help you with it. Whatever you choose to work on in the future be it OSS or a company environment will require being able to navigate these "softer" aspects of software development. The more you act in a way that makes people want to help you, the faster you will grow and the more opportunities you will have.
There was a problem hiding this comment.
I just wasn't satisfied with it as is so I provided an alternative that I believe to be better so that I didn't burden you
You just weren't satisfied with it and this is a valid reason to request changes?
I disagree with everything you wrote regarding cooperation and review and I think it is problematic that anyone thinks it is okay to order anyone based off their personal whims, caprices and personal philosophies especially in such an environment made to be collaborative such as open source.
I have pushed changes for this PR, maybe there is a better place to discuss this.
Chinwendu20
changed the title
IsUnique function to sliceHasDuplicates function to slice
Signed-off-by: Ononiwu Maureen <59079323+Chinwendu20@users.noreply.github.com>
| }, | ||
| { | ||
| name: "No items", | ||
| items: []int{}, |
There was a problem hiding this comment.
nit: there's no need to write out []int{} as nil evaluates to an empty slice.
|
Pushed a new tag |
Change Description
This PR adds a new function to check if the values in a slice have duplicates.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.馃摑 Please see our Contribution Guidelines for further guidance.