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

Insert existing doc into a new group #5

Merged
merged 6 commits into from
Jul 16, 2021

Conversation

bookis
Copy link
Contributor

@bookis bookis commented Jul 16, 2021

No description provided.

Copy link
Owner

@richardboehme richardboehme left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! I didn't even think about this particular issue before. I've added a comment about the current_position calculation. Maybe we could lazily evaluate it for now?

@@ -60,9 +60,11 @@ def move_to(position)
if position == 0
[nil, collection.first]
else
current_position = collection.map(&:id).index(self.id)
Copy link
Owner

Choose a reason for hiding this comment

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

Not in scope of this PR but I didn't like this solution in first place because it's operating on the collection within ruby which is very costly in terms of performance.

However, previously it was only called if needed. Now it is executed every time if the position is not 0. For example running the scope benchmark, that creates 5000 records, will run around ~15x slower than before (at least on my machine).

Maybe we can change it to only run if no_rank? is false. This would preserve the original behavior. I still need to figure out a way around this thing entirely...

Copy link
Owner

Choose a reason for hiding this comment

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

Well I just thought about it for 5 minutes and I think I have a solution:

The only case where we want to use collection.offset(position).limit(2) is when the item has a rank and is in front of the collection. We can just avoid all the checks and use

collection.where.not(id: self.id).offset(position - 1).limit(2)

The tests tell me that this seems to work. I'm going to merge your PR, make the aforementioned change and am going to release a new version of the gem!

@richardboehme richardboehme merged commit 1c59503 into richardboehme:main Jul 16, 2021
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.

2 participants