-
Notifications
You must be signed in to change notification settings - Fork 82
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @amab8901, thanks a lot for your investigation!
Sorry to nitpick with the EPSILON thingy; I don't want to risk anything and epsilon should be small enough not to cause any issues by matching too many documents.
thanks for your feedback, I applied it. Honestly, I wasn't even sure what I was doing. I just took pieces of code here and there that looked similar to what I was trying to do, and I combined them in places that seem reasonable, and it worked for the most part (except the things you mentioned in feedback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took pieces of code here and there that looked similar to what I was trying to do, and I combined them in places that seem reasonable, and it worked for the most part
Don't worry; that's how it goes when you start on a new codebase, you're doing great! 🌟
It seems like rustfmt is angry. Could you run it and push a new commit? cargo fmt
Then we're good to merge!
ok now I did
|
Ah yes, sorry that's definitely an issue on our side, and our Also, it look like you rebased on the wrong branch; you're now trying to merge way more commits than before 🤔 |
I fixed this one:
Only one of those commits are mine. The other ones come from other people and were introduced via |
Hey @amab8901, sorry I was on weekend, but I'm back now!
I don't really know what happened, but the easiest thing you can do is probably to follow these steps: git fetch origin # now origin/main` is up to date
git rebase -i origin/main
# that should open a window with a bunch of commits that are going to be rebased; you delete everything except your commits
git push --force # you need to force push because you're rewriting the git history and then we should be good to go 😁 |
f4d3936
to
0ce7eb8
Compare
how about now? I think I'm starting to become proficient with squashing. |
ok no problem, I'll just redo it and open a new PR. |
But why? Doing another PR won't change anything I think |
idk, I think I messed something up in the git. I ran the commands you proposed but it didn't get fixed |
Whatever you do, it looks like you lost the changes associated with your original PR, so please do not delete your milli directory. The changes are still there somewhere, and we can get them. Don't worry. We're going to debug this together. First, can you show me the result of; git remote show
# and then run; it will print the URL contained in all your remote
git remote show | xargs git remote show Maybe you don't have the original meilisearch as your |
|
Ok, so now I know why it failed! What we're going to do is create a second remote, so it's easier to refer to the commits coming from meilisearch. git remote add upstream https://github.com/meilisearch/milli.git # EDITED With this command, you now have the remote The next command is:
That's going to pull all the latest changes that happened on meilisearch.
And finally, we apply your commit to this branch and push:
That should put you back on track! 🚂 |
I find it interesting that I got this:
Otherwise, I made it to the end. I don't know, does it work now? I'm not really sure why the |
Oh gosh, yes sorry it was supposed to be milli 🤦 You can just run these commands to come back to the same state but with milli;
|
some highlights:
|
I think you forgot a bunch of commands here 🤔 Here is the list of commands you should run again
|
I ran all the commands. I shared only some interesting highlights rather than all of them. I particularly didn't share what happened if it's like empty output or something really simple and non-informative in output |
What was the output of;
Because it looks like this one didn't work 😬 |
oh, I had a typo there on This one Here's now the final output:
|
Nice! Now we're probably good to go, can you re-open this PR? |
Ah yeah, probably; go on, this time it should works! |
722: Geosearch for zero radius r=irevoire a=amab8901 # Pull Request ## Related issue Fixes #3167 (meilisearch/meilisearch#3167) ## What does this PR do? - allows Geosearch with zero radius to return the specified location when the coordinates match perfectly (instead of returning nothing). See link for more details. - new attempt on #713 ## PR checklist Please check if your PR fulfills the following requirements: - [ X ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [ X ] Have you read the contributing guidelines? - [ X ] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: amab8901 <amab8901@protonmail.com> Co-authored-by: Tamo <irevoire@protonmail.ch>
722: Geosearch for zero radius r=irevoire a=amab8901 # Pull Request ## Related issue Fixes #3167 (meilisearch/meilisearch#3167) ## What does this PR do? - allows Geosearch with zero radius to return the specified location when the coordinates match perfectly (instead of returning nothing). See link for more details. - new attempt on #713 ## PR checklist Please check if your PR fulfills the following requirements: - [ X ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [ X ] Have you read the contributing guidelines? - [ X ] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: amab8901 <amab8901@protonmail.com> Co-authored-by: Tamo <irevoire@protonmail.ch>
Pull Request
Related issue
Fixes #3167 (meilisearch/meilisearch#3167)
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!