-
Notifications
You must be signed in to change notification settings - Fork 4
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
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
bfa3021
add ability to move a record with an existing rank to a new group
bookis cbecd98
refactor group rank test and test helper to create docs
bookis a4f0532
refactor ranking method and group insert test
bookis 574a300
add test cases for inserting at top and end in new group
bookis ffabfa8
refactor Paragraph1 -> GroupedParagraph to avoid constant rename warn…
bookis 3d774d2
Merge pull request #1 from reflectivehq/move-group-to-insert
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
class GroupedParagraph < Paragraph | ||
rank!(group_by: :page_id) | ||
end | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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...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.
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 useThe 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!