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 original_scope method #313

Merged
merged 2 commits into from
Mar 24, 2023
Merged

Add original_scope method #313

merged 2 commits into from
Mar 24, 2023

Conversation

zhuravel
Copy link
Contributor

See discussion: 157986f#r105876563

Comment on lines +59 to +60
scope = original_scope
driver.to_scope(scope)
Copy link
Owner

Choose a reason for hiding this comment

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

local variable with the same name as method name can be confusing:

Suggested change
scope = original_scope
driver.to_scope(scope)
driver.to_scope(original_scope)

Copy link
Contributor Author

@zhuravel zhuravel Mar 24, 2023

Choose a reason for hiding this comment

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

It is not the same.

See commit with your suggestion: https://github.com/zhuravel/datagrid/commit/d5e2362c3f7e70da950260cb21e1eac044e47f73
See how it breaks build: https://github.com/zhuravel/datagrid/actions/runs/4512413662

This is due to order in which Ruby evaluates code (or some other reason that I can’t comprehend after years of writing Ruby code), original_scope has to be called before driver.to_scope and not passed as an argument. These options work though:

scope = original_scope
driver.to_scope(scope)
original_scope.then do |scope|
  driver.to_scope(scope)
end

lib/datagrid/core.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@bogdan bogdan merged commit 491470d into bogdan:master Mar 24, 2023
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