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 ActiveHash::Base.order method inspired by ActiveRecord #177

Merged
merged 7 commits into from Oct 26, 2019

Conversation

@Sean0628
Copy link
Contributor

Sean0628 commented Jul 20, 2019

This PR basically tries to add ActiveHash::Base.order class method.
Kindly consider my request, happy to take your feedback.
Thanks.

@syguer

This comment has been minimized.

Copy link
Collaborator

syguer commented Sep 28, 2019

@Sean0628
Sorry for late reply 🙇
Your idea looks nice! but a point I was concerning long time is iwe can't use it same as .order on ActiveRecod.
Generally, .order is used .where together. This implementation doesn't satisfied it.
But we know it was difficult on ActiveHash because ActiveHash#where doesn't return chainable value.
But today, it is fixed on #178. So I suggest 2 ways

  1. (I'm afraid but) Re-implement to follow chainable
  2. This PR merge to stable-2.x and release as 2.4.0 (After it, I will do 1)
@Sean0628

This comment has been minimized.

Copy link
Contributor Author

Sean0628 commented Sep 28, 2019

@syguer
Thank you for the replay 🙇

I am down for no1 idea.

(I'm afraid but) Re-implement to follow chainable

To be honest, the point you mentioned was my concern too. It was real tough at the same time like you said.

However, #178 has been merged. So, now I am gladly to make this chainable. It might take a couple of days though.

Thank you.

@Sean0628

This comment has been minimized.

Copy link
Contributor Author

Sean0628 commented Sep 28, 2019

@syguer
Added support for .where chain. Would love to hear your feedback.
Thanks.


order_by_args!(candidates, processed_args)

candidates

This comment has been minimized.

Copy link
@nsommer

nsommer Sep 30, 2019

Contributor

This seems to returns an Array, but I think it would be better to return an ActiveHash::Relation again, so that you can chain it.

To make that work, I think instead of returning the duped & ordered records, writing them back to @records and returning self should do it.

This comment has been minimized.

Copy link
@Sean0628

Sean0628 Oct 1, 2019

Author Contributor

@nsommer
Thanks for your comment.

This method returned an Array object when I first implemented as you mentioned.
However, it does return ActiveHash::Relation after this change cf.fcfa0ab.

Country.order("code, id DESC").is_a?(ActiveHash::Relation)
=> true
Country.where(language: 'English').order(id: :desc).is_a?(ActiveHash::Relation)
=> true

This returns ActiveHash::Relation so that it can be used like following example.

Country.order("code, id DESC").where(id: 1)

Does this resolve your concern?
Let me know if you need further explanation. Would love to share it.

Thanks!

This comment has been minimized.

Copy link
@nsommer

nsommer Oct 1, 2019

Contributor

Oh, I see. I was confused because of the local records variable which gets a relation assigned (with where({})), because there is also a private method records which returns the data array. Maybe you can rename the local variable to something like relation_to_order or something like that?

This comment has been minimized.

Copy link
@Sean0628

Sean0628 Oct 1, 2019

Author Contributor

I was confused because of the local records variable which gets a relation assigned (with where({})), because there is also a private method records which returns the data array.

Thank you for your feedback!
This makes a lot of sense to me. records was a bit confusing.
Renamed records to relation.

This comment has been minimized.

Copy link
@nsommer

nsommer Oct 2, 2019

Contributor

Great 👍

Sean0628 and others added 2 commits Oct 1, 2019
@syguer

This comment has been minimized.

Copy link
Collaborator

syguer commented Oct 26, 2019

Sorry for late.
Looks good! Thank you 😄

@syguer syguer merged commit 9ebc83d into zilkey:master Oct 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sean0628 Sean0628 deleted the Sean0628:add_order_method branch Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.