Skip to content

Commit

Permalink
Naming is hard 😬😅
Browse files Browse the repository at this point in the history
  • Loading branch information
@jonatack
jonatack committed Sep 30, 2016
1 parent bb2af90 commit ae23753
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 48 deletions.
40 changes: 16 additions & 24 deletions spec/mongoid/configuration_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,13 @@ module Ransack
end

it 'changes default search key parameter' do
# store original state so we can restore it later
before = Ransack.options.clone
default = Ransack.options.clone

Ransack.configure do |config|
config.search_key = :query
end
Ransack.configure { |c| c.search_key = :query }

expect(Ransack.options[:search_key]).to eq :query

# restore original state so we don't break other tests
Ransack.options = before
Ransack.options = default
end

it 'should have default values for arrows' do
Expand All @@ -55,35 +51,31 @@ module Ransack
end

it 'changes the default value for the up arrow only' do
before = Ransack.options.clone
new_up_arrow = 'U+02191'
previous_down_arrow = before[:down_arrow]
default, new_up_arrow = Ransack.options.clone, 'U+02191'

Ransack.configure { |c| c.custom_arrows = { up_arrow: new_up_arrow } }

expect(Ransack.options[:down_arrow]).to eq default[:down_arrow]
expect(Ransack.options[:up_arrow]).to eq new_up_arrow
expect(Ransack.options[:down_arrow]).to eq previous_down_arrow

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016 •
edited
Loading

Author Contributor

The previous_down_arrow naming was a mistake on my part. It's the unmodified default down arrow. I became confused by the before naming, conflating 'before' with 'previous'. Changing the options local var naming from 'before' to 'default' gives us default[:down_arrow], which IMHO expresses its identity both correctly and clearly enough that no intermediary local var is needed for clarity.

This comment has been minimized.

Sign in to view

Copy link
@garettarrowood

garettarrowood Sep 30, 2016

Contributor

Agreed. This appears to be the cleanest implementation yet.

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016

Author Contributor

Something about it was bothering me, but it took a few iterations to find it 😬. Thanks for the review and feedback - I appreciate it!

Ransack.options = before
Ransack.options = default
end

it 'changes the default value for the down arrow only' do
before = Ransack.options.clone
previous_up_arrow = before[:up_arrow]
new_down_arrow = '<i class="down"></i>'
default, new_down_arrow = Ransack.options.clone, '<i class="down"></i>'

Ransack.configure { |c| c.custom_arrows = { down_arrow: new_down_arrow } }

expect(Ransack.options[:up_arrow]).to eq previous_up_arrow
expect(Ransack.options[:up_arrow]).to eq default[:up_arrow]

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016 •
edited
Loading

Author Contributor

Same thing here.
expect(Ransack.options[:down_arrow]).to eq new_down_arrow

Ransack.options = before
Ransack.options = default
end

it 'changes the default value for both arrows' do
before = Ransack.options.clone
new_up_arrow = '<i class="fa fa-long-arrow-up"></i>'
new_down_arrow = 'U+02193'
default = Ransack.options.clone
new_up_arrow = '<i class="fa fa-long-arrow-up"></i>'
new_down_arrow = 'U+02193'

Ransack.configure do |c|
c.custom_arrows = { up_arrow: new_up_arrow, down_arrow: new_down_arrow }
Expand All @@ -92,14 +84,14 @@ module Ransack
expect(Ransack.options[:up_arrow]).to eq new_up_arrow
expect(Ransack.options[:down_arrow]).to eq new_down_arrow

Ransack.options = before
Ransack.options = default
end

it 'consecutive arrow customizations respect previous customizations' do
before = Ransack.options.clone
default = Ransack.options.clone

Ransack.configure { |c| c.custom_arrows = { up_arrow: 'up' } }
expect(Ransack.options[:down_arrow]).to eq before[:down_arrow]
expect(Ransack.options[:down_arrow]).to eq default[:down_arrow]

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016

Author Contributor

default[:down_arrow] is clearer to me than before[:down_arrow].

Ransack.configure { |c| c.custom_arrows = { down_arrow: 'DOWN' } }
expect(Ransack.options[:up_arrow]).to eq 'up'
Expand All @@ -110,7 +102,7 @@ module Ransack
Ransack.configure { |c| c.custom_arrows = { down_arrow: 'down arrow-2' } }
expect(Ransack.options[:up_arrow]).to eq '<i>U-Arrow</i>'

Ransack.options = before
Ransack.options = default
end

it 'adds predicates that take arrays, overriding compounds' do
Expand Down
40 changes: 16 additions & 24 deletions spec/ransack/configuration_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,13 @@ module Ransack
end

it 'changes default search key parameter' do
# store original state so we can restore it later
before = Ransack.options.clone
default = Ransack.options.clone

Ransack.configure do |config|
config.search_key = :query
end
Ransack.configure { |c| c.search_key = :query }

expect(Ransack.options[:search_key]).to eq :query

# restore original state so we don't break other tests
Ransack.options = before
Ransack.options = default
end

it 'should have default values for arrows' do
Expand All @@ -55,35 +51,31 @@ module Ransack
end

it 'changes the default value for the up arrow only' do
before = Ransack.options.clone
new_up_arrow = 'U+02191'
previous_down_arrow = before[:down_arrow]
default, new_up_arrow = Ransack.options.clone, 'U+02191'

Ransack.configure { |c| c.custom_arrows = { up_arrow: new_up_arrow } }

expect(Ransack.options[:down_arrow]).to eq default[:down_arrow]
expect(Ransack.options[:up_arrow]).to eq new_up_arrow
expect(Ransack.options[:down_arrow]).to eq previous_down_arrow

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016 •
edited
Loading

Author Contributor

Same here.

Ransack.options = before
Ransack.options = default
end

it 'changes the default value for the down arrow only' do
before = Ransack.options.clone
previous_up_arrow = before[:up_arrow]
new_down_arrow = '<i class="down"></i>'
default, new_down_arrow = Ransack.options.clone, '<i class="down"></i>'

Ransack.configure { |c| c.custom_arrows = { down_arrow: new_down_arrow } }

expect(Ransack.options[:up_arrow]).to eq previous_up_arrow
expect(Ransack.options[:up_arrow]).to eq default[:up_arrow]

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016

Author Contributor

And here.
expect(Ransack.options[:down_arrow]).to eq new_down_arrow

Ransack.options = before
Ransack.options = default
end

it 'changes the default value for both arrows' do
before = Ransack.options.clone
new_up_arrow = '<i class="fa fa-long-arrow-up"></i>'
new_down_arrow = 'U+02193'
default = Ransack.options.clone
new_up_arrow = '<i class="fa fa-long-arrow-up"></i>'
new_down_arrow = 'U+02193'

Ransack.configure do |c|
c.custom_arrows = { up_arrow: new_up_arrow, down_arrow: new_down_arrow }
Expand All @@ -92,14 +84,14 @@ module Ransack
expect(Ransack.options[:up_arrow]).to eq new_up_arrow
expect(Ransack.options[:down_arrow]).to eq new_down_arrow

Ransack.options = before
Ransack.options = default
end

it 'consecutive arrow customizations respect previous customizations' do
before = Ransack.options.clone
default = Ransack.options.clone

Ransack.configure { |c| c.custom_arrows = { up_arrow: 'up' } }
expect(Ransack.options[:down_arrow]).to eq before[:down_arrow]
expect(Ransack.options[:down_arrow]).to eq default[:down_arrow]

This comment has been minimized.

Sign in to view

Copy link
@jonatack

jonatack Sep 30, 2016 •
edited
Loading

Author Contributor

And here.

Ransack.configure { |c| c.custom_arrows = { down_arrow: 'DOWN' } }
expect(Ransack.options[:up_arrow]).to eq 'up'
Expand All @@ -110,7 +102,7 @@ module Ransack
Ransack.configure { |c| c.custom_arrows = { down_arrow: 'down arrow-2' } }
expect(Ransack.options[:up_arrow]).to eq '<i>U-Arrow</i>'

Ransack.options = before
Ransack.options = default
end

it 'adds predicates that take arrays, overriding compounds' do
Expand Down

1 comment on commit ae23753

@jonatack
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Related to #726, but not only).

Please sign in to comment.