leikind / wice_grid

A Rails grid plugin to create grids with sorting, pagination, and (automatically generated) filters
MIT License
536 stars 215 forks source link

frozen string and num_pages #316

Closed Datyv closed 7 years ago

Datyv commented 7 years ago

changed num_pages to total_pages (deprecated) remove a line that conflicts with # frozen_string_literal: true

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.02%) to 37.567% when pulling fdc5d6304622a0fbad25e3cd0651c1c95652942a on Datyv:frozen-string into 1907e565f6850515595e18ae1581ef3129d0e9de on leikind:rails3.

leikind commented 7 years ago

why would opts[:order_direction] be frozen?

Datyv commented 7 years ago

when the order_direction declaration in the view-controller is frozen. is there any szenario where the order_direction is case-insensitive?

leikind commented 7 years ago

So it's about string literals immutability in 2.3. Ok.

And your solution is remove this. That is not a solution. Do you see that a few lines below the code relies on opts[:order_direction] being downcased?

Removing a line is not a solution. The solution is to create a new downcases String object and assign it to opts[:order_direction]

Datyv commented 7 years ago

oh .. you're right. I completly overread that part. Shame. Is this better ?

order_direction = opts[:order_direction]
order_direction.downcase! if order_direction.is_a?(String)

# validate :order_direction
if order_direction && ! (order_direction == 'asc' || order_direction == :asc || order_direction == 'desc' ||
                                      order_direction == :desc)
   raise WiceGridArgumentError.new(":order_direction must be either 'asc' or 'desc'.")
end
leikind commented 7 years ago

No

opts[:order_direction] = opts[:order_direction].downcase
Datyv commented 7 years ago

Sorry bothering you with this, but sadly this won't work. Still throws the same error. But this works like a charm: if opts[:order_direction] && ! (opts[:order_direction].to_s.casecmp('asc') == 0 || opts[:order_direction].to_s.casecmp('desc') == 0) raise WiceGridArgumentError.new(":order_direction must be either 'asc' or 'desc'.") end

leikind commented 7 years ago

What do do you mean this won't work.

irb(main):001:0> a = "ASC"
"ASC"
irb(main):002:0> a.freeze
"ASC"
irb(main):003:0> a.frozen?
true
irb(main):004:0> a = a.downcase
"asc"
irb(main):005:0> a.frozen?
false
coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.04%) to 37.585% when pulling 153b0e86a2e24ee772a4a76c22a8db76f3e77a20 on Datyv:frozen-string into 1907e565f6850515595e18ae1581ef3129d0e9de on leikind:rails3.

Datyv commented 7 years ago

oh yeah.. didn't see the missing bang, sorry

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 37.549% when pulling e9ab412723934d4a47e97dba01219a841d94c834 on Datyv:frozen-string into 1907e565f6850515595e18ae1581ef3129d0e9de on leikind:rails3.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 37.549% when pulling 2679a83530ceb11a7d09d90f36f6382ccc401652 on Datyv:frozen-string into 1907e565f6850515595e18ae1581ef3129d0e9de on leikind:rails3.