Define Hash#to_query and set Hash#to_param as alias to it; with test cases

[akshay-vishnoi]

No description provided.

[egilburg]

why do you need this require here?

[akshay-vishnoi]

@egilburg - Please check the updated code.

[egilburg]

🆗

[akshay-vishnoi]

I think if Hash#to_param is dependent on to_query then it should require to_query. Please check my Updated PR.

[egilburg]

1. requires are conventionally put at top of file, not bottom.
2. to_query file already requires to_param. Mutual requires are usually not a good design.
3. Current design doesn't make to_param dependent on to_query but rather the other way around (to_query.rb says that hash.to_query should behave as already-defined hash.to_param, while hash.to_param always works due to code in to_param.rb, even if to_query.rb file isn't loaded at all).

[akshay-vishnoi]

1. Yes conventionally we put them at top, but we can also require them at other places.
2. Read point 3.
3. please look at https://github.com/akshay-vishnoi/rails/blob/test-to_param/activesupport/lib/active_support/core_ext/object/to_param.rb#L56, to_param is also calling to_query.

[egilburg]

Update: I realized that Hash code in to_param.rb does indeed call to_query on line 56. So there is an implicit dependency there.

[akshay-vishnoi]

Yes.. 😄

[egilburg]

I wonder if better design then would be to move Hash's to_param definition to be in to_query.rb instead (because it obviously doesn't work without to_query.rb, so no point trying to make it seem like its separate concept), and redefine as def to_query, and then switch the alias around:

In to_query.rb:

# other classes as before

class Hash
  # Returns a string representation of the receiver suitable for use as a URL
  # query string:
  #
  #   {name: 'David', nationality: 'Danish'}.to_query
  #   # => "name=David&nationality=Danish"
  #
  # An optional namespace can be passed to enclose the key names:
  #
  #   {name: 'David', nationality: 'Danish'}.to_query('user')
  #   # => "user[name]=David&user[nationality]=Danish"
  #
  # The string pairs "key=value" that conform the query string
  # are sorted lexicographically in ascending order.
  #
  # This method is also aliased as +to_param+.
  def to_query(namespace = nil)
    collect do |key, value|
      unless (value.is_a?(Hash) || value.is_a?(Array)) && value.empty?
        value.to_query(namespace ? "#{namespace}[#{key}]" : key)
      end
    end.compact.sort! * '&'
  end

  alias_method :to_param, :to_query
end

And remove the Hash#to_param definition from to_param.rb

That way to_param.rb remains independent from to_query.rb and the relationship between the two files is cleaner.

[akshay-vishnoi]

👍 , I think you are right. But instead of defining the method in to_query.rb, we can shift line 31 to to_param.rb. WDYT?

[egilburg]

it wont solve problem of to_param and to_query both depending on code in each other. mutual dependencies are generally not good when they can be avoided

[akshay-vishnoi]

Hey I think to_param.rb is a public API, so when you will move to_param from this file, then Any App using this file will break.

[akshay-vishnoi]

cc: @senny, @rafaelfranca.

[egilburg]

If apps load both files, behavior should be as-is (Hash#to_param will be aliased to #to_query by to_query.rb), which is no different than current behavior where Hash#to_query is aliased to Hash#to_param).

If app for whatever reason loads only the to_param.rb file (that's not supported by Rails API anyways), than the behavior will differ in sense that Hash#to_param will fall back to Object#to_param which by default does #to_s, while currently it will raise a NoMethodError because it'll try to call to_query which is undefined if to_query.rb isn't loaded.

I don't think any of the above is an API concern. It's more about cleaner and more maintainable code with no implicit or mutual dependencies.

[akshay-vishnoi]

Its correct. I think we are now in condition for some suggestions.

[akshay-vishnoi]

@senny : Could You also take a look at this please?

[akshay-vishnoi]

Hi any updates?

[rafaelfranca]

This would cause circular dependency. For me this implementation should be moved to to_query and this one be an alias for it.

[akshay-vishnoi]

I have updated the Code, please check again.

[rafaelfranca]

My problem with this patch is if users only include to_param they will not get Hash#to_param 😢

I think the implementation before this patch is the only one that satisfy all the requirement. But for understand better, what is the motivation of this change?

[egilburg]

Hash.to_param would still work by virtue of Object.to_param. Yes, it'll produce hash.to_s which will probably be not useful to anyone.

The motivation is a hidden dependency where code in to_param requires code in to_query, on top of the explicit dependnecy where code in to_query requires to_param. So there's already an implicit circular dependenency, and I wonder if it can be cleaned up.

[rafaelfranca]

I see. It is trick. I don't think we have a proper way to fix. Right now the situation is worse than before. Using Object.to_param is way worse than receiving a exception of missing to_query method.

I think is better to accept the hidden dependency. Do you have any more suggestion?

[egilburg]

Not really, without level of abstraction/composition which won't be worth the complexity. Except perhaps explicitly require the other class as well.

[akshay-vishnoi]

If we stay with previous implementation, then the same question you asked will come i.e.
if users only include to_param then they will get an exception on calling Hash#to_param, which is again you won't get the purpose of to_param. So in any case we can't include only to_param, because it will not behave properly on Hash#to_param. So why not atleast move Hash#to_param to proper place?

[rafaelfranca]

I think the best option make both files require the same file and have the implementation in the same place.

[akshay-vishnoi]

Also shouldn't we remove split('&') from here? I think its not required.

[akshay-vishnoi]

Code updated. Please review.

[akshay-vishnoi]

Hey its been a month, any progress on this issue?