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

Provide friendlier access to request variants #18939

Merged
merged 2 commits into from Mar 27, 2015

Conversation

@georgeclaghorn
Copy link
Member

georgeclaghorn commented Feb 14, 2015

Currently, request.variant is a dumb reader for the underlying array used to store the request's variants. If you set the variant like so:

request.variant = :phone

...then request.variant will return [:phone]. That can be cumbersome to deal with, since you have to check request.variant.first == :phone, request.variant == [:phone], request.variant.to_a.include?(:phone), etc.

This PR implements a friendlier interface:

request.variant = :phone
request.variant.phone?   # true
request.variant.tablet?  # false

request.variant = [:phone, :tablet]
request.variant.phone?    # true
request.variant.tablet?   # true
request.variant.desktop?  # false
request.variant.any?(:phone, :desktop)  # true
request.variant.any?(:desktop, :watch)  # false

Fixes #18933.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch Feb 14, 2015
@kaspth
kaspth reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
(variants & candidates).any?
end

def to_ary

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

What is it this gets us?

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

This allows request.variant to be compared to other arrays, since Array comparison methods like #==, #&, and #| call to_ary on the other array:

request.variant = :phone
# => #<VariantInquirer @variants=[:phone]>

[:phone] == request.variant
# => true

Existing framework code relies on this behavior in a few places.

@simi
simi reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
@@ -139,6 +141,33 @@ def negotiate_mime(order)
order.include?(Mime::ALL) ? format : nil
end

class VariantInquirer # :nodoc:

This comment has been minimized.

Copy link
@simi

simi Feb 14, 2015

Contributor

What about to introduce ArrayInquirer to ActiveSupport? There is already similar code for String.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

That'd be a nice generalization, but I think it should be accomplished in a separate PR. As it is, this is specific to the array of Symbols used to store variants.

This comment has been minimized.

Copy link
@jvanbaarsen

jvanbaarsen Feb 22, 2015

Contributor

I like the name of this class 👍
--edit: But after reading the PR discussion I see this gets extracted to a ArrayInquirer :D

@lucasmazza
lucasmazza reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
if variant.is_a?(Symbol)
@variant = [variant]
elsif variant.nil? || variant.is_a?(Array) && variant.any? && variant.all?{ |v| v.is_a?(Symbol) }
if variant.nil?
@variant = variant

This comment has been minimized.

Copy link
@lucasmazza

lucasmazza Feb 14, 2015

Member

Now that we have the VariantInquirer class, might make sense to initialize the @variant ivar with an empty VariantInquirer instance, otherwise the client code would have to check if request.variant is nil before calling the query methods like phone? or tablet?.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

I might be mistaken here, but I think these if statements can be simplified as:

variants = Array(variants).compact

if variants.all? { |v| v.is_a?(Symbol) }
  @variant = VariantInquirer.new(variants)
else
  raise ArgumentError, "super long message"
end

Though I don't know how much of a hot spot this code path is.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

Ah, Array(nil) returns an empty array, so there's no need for the compact call.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

Just continuing my own ping-pong. It could also be:

@variant = VariantInquirer.new Array(variant).tap do |variants|
  unless variants.all? { |v| v.is_a?(Symbol) }
    raise ArgumentError, "super long message"
  end
end

This comment has been minimized.

Copy link
@lucasmazza

lucasmazza Feb 14, 2015

Member

👍 for the non-tap version

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

Done.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch Feb 14, 2015
@simi
simi reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
class VariantInquirer # :nodoc:
delegate :==, :each, :empty?, to: :variants

def initialize(variants)

This comment has been minimized.

Copy link
@simi

simi Feb 14, 2015

Contributor

variants = [] to avoid VariantInquirer.new([]) and use VariantInquirer.new instead

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

👍

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch Feb 14, 2015
@simi
simi reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
end

private
attr_reader :variants

This comment has been minimized.

Copy link
@simi

simi Feb 14, 2015

Contributor

This should be defined on the top of its scope IMHO.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

IMO, it shouldn't. It's intended for internal use only. Doesn't make sense to call request.variant.variants.

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Feb 14, 2015

Member

It should be made protected then, private attr accessors raise ruby warnings.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

TIL! Fixed.

@kaspth
kaspth reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
else
raise ArgumentError, "request.variant must be set to a Symbol or an Array of Symbols, not a #{variant.class}. " \
raise ArgumentError, "request.variant must be set to a Symbol or an Array of Symbols. " \

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

I don't know if we can just get rid of outputting the class.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

I'll put it back. It seemed awkward that if you set request.variant to an array of non-Symbol objects, you'd see:

request.variant must be set to a Symbol or an Array of Symbols, not a Array.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

You know what, leave it 😄

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

I already put it back. 😄

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 14, 2015

Member

I know, I saw. Sorry about that. 😉

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch Feb 14, 2015
@egilburg
egilburg reviewed Feb 14, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
attr_reader :variants

def method_missing(name, *args)
if name[-1] == '?'

This comment has been minimized.

Copy link
@egilburg

egilburg Feb 14, 2015

Contributor

name.end_with?('?')

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 14, 2015

Author Member

name is a symbol. No #end_with? method.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch 3 times, most recently Feb 14, 2015
@simi
simi reviewed Feb 15, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
end

protected
attr_reader :variants

This comment has been minimized.

Copy link
@simi

simi Feb 15, 2015

Contributor

I still wonder why this can't be public since you can access it via to_ary also.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 15, 2015

Author Member

It shouldn't be public because request.variant.variants is an awkward and opaque invocation for accessing the variants as an array.

I'd be open to exposing it publicly as to_a.

This comment has been minimized.

Copy link
@simi

simi Feb 15, 2015

Contributor

actually request.variant.to_a is weird to me (typing singular to array)

What about to use another name for this array? What about request.variant.candidates or something similar?

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 15, 2015

Author Member

It doesn't really seem that weird to me. It's an object that can be converted to a plain-old array. Anyway, what's the case where you would need access to the array? I tried to think of one and drew a blank.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch Feb 15, 2015
@egilburg
egilburg reviewed Feb 15, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
@@ -139,6 +141,31 @@ def negotiate_mime(order)
order.include?(Mime::ALL) ? format : nil
end

class VariantInquirer # :nodoc:
delegate :==, :each, :empty?, to: :@variants

This comment has been minimized.

Copy link
@egilburg

egilburg Feb 15, 2015

Contributor

Perhaps == should be delegated to @variants in globbed form:

def ==(*variants)
  @variants == variants
end

Before:
request.variant == :tablet # will always return false so probably not something user wants

After:
request.variant == :tablet # will return true if tablet is the exact match and no other variants exist.

This comment has been minimized.

Copy link
@egilburg

egilburg Feb 15, 2015

Contributor

Alternatively if exact match is not something we need in api, then we can just alias == to any?. IMO either option would be more consistent than just delegating == without globbing.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 15, 2015

Author Member

With globbing:

request.variant = [:phone, :tablet]
request.variant == [:phone, :tablet]
# => false

Perhaps the solution here is to do something similar to what we do when assigning request.variant:

def ==(other)
  @variants == Array(other)
end

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 15, 2015

Member

I still don't understand why we need #==. The point of this is to provide a better interface, i.e. #any?.

Are there places in the framework that use == with variants?

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 15, 2015

Author Member

I thought more tests failed without it, but it seems that only one does, and that's because the test itself uses assert_equal to compare response.variant with a symbol. I'm fine with removing it.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 15, 2015

Member

I don't know if that's commonly used. But we could do a deprecation cycle to be safe.

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 15, 2015

Author Member

Couldn't find any public apps on Github using #==, so I just removed it. I don't think it's worth a deprecation cycle.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 15, 2015

Member

Nice and well done 👍

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Feb 15, 2015

Author Member

For what it's worth, I kept looking and found two apps using #== in the first 15 pages of Ruby code search results on GitHub:

Not sure if this changes whether we should leave #== in. It's clearly not widely used, but it makes me nervous that it could silently change behavior for the few apps that do rely on it.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 16, 2015

Member

Let's do a deprecation cycle, so people will have an easier time updating.

@egilburg
egilburg reviewed Feb 15, 2015
View changes
actionpack/lib/action_dispatch/http/mime_negotiation.rb Outdated
class VariantInquirer # :nodoc:
delegate :==, :each, :empty?, to: :@variants

def initialize(variants = [])

This comment has been minimized.

Copy link
@egilburg

egilburg Feb 15, 2015

Contributor

If supporting == operator, perhaps cast this to Set for order-insensitive comparison.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch 4 times, most recently Feb 15, 2015
@kaspth kaspth added this to the 5.0.0 milestone Feb 15, 2015
@kaspth kaspth added the actionpack label Feb 15, 2015
@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch Feb 15, 2015
@egilburg
Copy link
Contributor

egilburg commented Feb 16, 2015

@dhh would an ArrayInquirer also imply an ArrayWithIndifferentAccess? or do a runtime cast to_s or to_str?

e.g. in this example:

class SoftString
  def to_s
    'soft'
  end
end

class HardString
  def to_str
    'hard'
  end
end

ai = ActiveSupport::ArrayInquirer.new(['foo', :bar, SoftString.new, HardString.new])

What should each of these return? true, false, or cast exception?

ai.foo?
ai.bar?
ai.soft?
ai.hard?

And BTW this is not current behavior of StringInquirer:

ActiveSupport::StringInquirer.new(:foo)
# => TypeError: can't convert Symbol into String

It only works for Strings and objects which implement to_str, which Symbol doesn't. And this particular PR works on Symbols, so a similar implementation of ArrayInquirer won't work here - presumably, it would raise an exception during ArrayInquirer initialization, before even having a chance to call the method missing.

(perhaps for consistency, StringInquirer) should special-case handling symbols without raising, by casting them to_s? It could also handle any other object by to_s match but the latter could presumably be more disruptive for existing users.

@dhh
Copy link
Member

dhh commented Feb 16, 2015

I'm thinking it's a runtime comparison. So it works with both Indifferent and not. I also think that we should have a SymbolInquery. I've wanted that in the past.

@egilburg
Copy link
Contributor

egilburg commented Feb 16, 2015

@dhh do you prefer two separate inquirer classes, one just for strings and one just for symbols, rather than soften StringInquirer to accept symbols (and just cast them to_s on initialize)?

@dhh
Copy link
Member

dhh commented Feb 16, 2015

I was thinking about StringInquirer taking a symbol, but then it’s not a symbol any more, it’s a string. I’d rather just be explicit and have a SymbolInquirer that inherits from Symbol.

On Feb 16, 2015, at 11:18, Eugene Gilburg notifications@github.com wrote:

@dhh do you prefer two separate inquirer classes, one just for strings and one just for symbols, rather than soften StringInquirer to accept symbols (and just cast them to_s on initialize)?


Reply to this email directly or view it on GitHub.

@dhh
Copy link
Member

dhh commented Feb 16, 2015

Anyway, let's split those off into their own tickets. Don't want to swamp this ticket with those concerns. I'm very eager to get this in sooner rather than later.

@georgeclaghorn
Copy link
Member Author

georgeclaghorn commented Mar 12, 2015

Updated to use an extracted ArrayInquirer.

@dhh @kaspth

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch 4 times, most recently from 979ea43 to 3936658 Mar 12, 2015

def method_missing(name, *args)
if name[-1] == '?'
any?(name[0..-2]) || any?(name[0..-2].to_sym)

This comment has been minimized.

Copy link
@egilburg

egilburg Mar 12, 2015

Contributor

for consistency between string/symbol, this check should be at the any? level:

def any?(*candidates, &block)
  if candidates.none?
    super
  else
    candidates.detect do |candidate|
      include?(candidate) || include?(candidate.to_sym)
    end || false
  end
end 

private

def method_missing(name, *args)
  if name[-1] == '?'
    any?(name[0..2])
  end
end

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 12, 2015

Member

Your suggested code changes are a little off. We don't need to check for none? or explicitly return false. But I agree with moving the check 👍

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Mar 14, 2015

Author Member

Fixed.

# its string-like contents. For example, <tt>request.variant</tt> returns an
# +ArrayInquirer+ object. To check a request's variants, you can call:
#
# request.variant.phone?

This comment has been minimized.

Copy link
@egilburg

egilburg Mar 12, 2015

Contributor

We aren't dealing with request here, make explicit usage example:

  # variant = ArrayInquirer.new(:phone, :tablet, :desktop)
  #
  # variant.phone? # => true
  # variant.laptop? # => false
  #
  # variant.any?(:phone, :desktop) # => true
  # variant.any?(:phone, :laptop) # => true
  # variant.any?(:laptop, :desktop) # => false

This comment has been minimized.

Copy link
@georgeclaghorn

georgeclaghorn Mar 14, 2015

Author Member

Fixed.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch 2 times, most recently from 0dd8cbe to 392b3e7 Mar 14, 2015

variants.any?(:phone, :tablet) # => true
variants.any?(:phone, :desktop) # => true
variants.any?(:desktop, :watch) # => false

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 14, 2015

Member

Clever thinking with the watch here 😄

@kaspth
Copy link
Member

kaspth commented Mar 14, 2015

Looks great to me, 👍

assert_not @array_inquirer.any? { |v| v == :desktop }
end

def test_respond_to

This comment has been minimized.

Copy link
@egilburg

egilburg Mar 14, 2015

Contributor

assert the shortcut method since you added it:

def test_inquiry_method
  assert_equal @array_inquirer, [:mobile, :tablet].inquiry
end
Wrapping an array in an `ArrayInquirer` gives a friendlier way to check its
string-like contents. For example, `request.variant` returns an `ArrayInquirer`
object. To check a request's variants, you can call:

    request.variant.phone?
    request.variant.any?(:phone, :tablet)

...instead of:

    request.variant.include?(:phone)
    request.variant.any? { |v| v.in?([:phone, :tablet]) }

`Array#inquiry` is a shortcut for wrapping the receiving array in an
`ArrayInquirer`:

    pets = [:cat, :dog]
    pets.cat?    # => true
    pets.ferret? # => false
    pets.any?(:cat, :ferret} # => true
@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:variant-inquiry branch from 392b3e7 to c64b99e Mar 24, 2015
@rafaelfranca rafaelfranca merged commit c64b99e into rails:master Mar 27, 2015
rafaelfranca added a commit that referenced this pull request Mar 27, 2015
Provide friendlier access to request variants
@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 27, 2015

Thank you so much. The code is great, but I'd not include the Array#inquiry we are promoting too much a feature that I don't think it will be used in most part of the applications. Lets keep just the ArrayInquirer constructor.

@dhh
Copy link
Member

dhh commented Mar 28, 2015

Great work, George! 🤘

@dhh
Copy link
Member

dhh commented Mar 28, 2015

I think the ArrayInquirer makes as much sense as the StringInquirer. I'm certainly going to use it. I think it's fair to highlight that it's there.

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 30, 2015

I mean, it will still be highlighted in the CHANGELOG and documentation but I don't think it needs a Array#inquiry method. Even that we have a lot of usage of this feature it will be usually well encapsulated, so we can call ActiveSupport::ArrayInquirer.new without losing the legibility.

@georgeclaghorn georgeclaghorn deleted the georgeclaghorn:variant-inquiry branch Mar 30, 2015
@dhh
Copy link
Member

dhh commented Mar 30, 2015

I like the #inquiry form, as it makes it possible to use this inline on a passed parameter without extra setup. And it then maps directly to String#inquiry as well.

On Mar 30, 2015, at 3:56 AM, Rafael Mendonça França notifications@github.com wrote:

I mean, it will still be highlighted in the CHANGELOG and documentation but I don't think it needs a Array#inquiry method. Even that we have a lot of usage of this feature it will be usually well encapsulated, so we can call ActiveSupport::ArrayInquirer.new without losing the legibility.


Reply to this email directly or view it on GitHub.

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 30, 2015

👍, so I'll add back

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 30, 2015

Add back at b5c3502

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

10 participants
You can’t perform that action at this time.