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

Add makeNonTrivial helper in Search #5092

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kunaltyagi
Copy link
Member

@kunaltyagi kunaltyagi commented Dec 19, 2021

Instead of modifying multiple call sites with what looks like really weird code, I think it's better to instead add a helper and test it properly.
Related PR: #5051 #5057

@kunaltyagi kunaltyagi force-pushed the search.nonTrivial branch 2 times, most recently from b16785b to 22e8f70 Compare December 20, 2021 04:02
@kunaltyagi kunaltyagi added changelog: new feature Meta-information for changelog generation module: search labels Dec 20, 2021
@kunaltyagi kunaltyagi marked this pull request as ready for review December 21, 2021 04:06
@kunaltyagi kunaltyagi requested review from larshg and mvieth December 21, 2021 04:06
@kunaltyagi
Copy link
Member Author

Candidate for squash-merge if accepted

@kunaltyagi
Copy link
Member Author

Ping

@larshg
Copy link
Contributor

larshg commented Feb 11, 2022

I don't quite get the name makeNonTrivial?
Except for that, I think its makes good sense to have this login here and the use these calls whenever we are using the radius search and the like?

@kunaltyagi
Copy link
Member Author

Ah, yeah, I couldn't think of a shorter name.

Returning the same point as result of search is basically a trivial answer. Ofc the point from the cloud is going to be in there.
So i thought, removePoint, removeSearchPoint, might be confusing, and making the search result non-trivial might not be.

Maybe I overthought? 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: new feature Meta-information for changelog generation module: search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants