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

Refactor UserController#search_users #57

Merged
merged 6 commits into from Feb 7, 2013

Conversation

7 participants
@blowmage
Contributor

blowmage commented Feb 7, 2013

Move the SQL generation behind an intention revealing name, add tests.

@eviltrout

This comment has been minimized.

Show comment
Hide comment
@eviltrout

eviltrout Feb 7, 2013

Member

Love it. Any idea why it doesn't merge cleanly? Also would you mind filling out our CLA: http://www.discourse.org/cla

@SamSaffron this approach keeps the SQL but isolates it from the controller. I vastly prefer it but would be more comfortable with your approval before merging.

Member

eviltrout commented Feb 7, 2013

Love it. Any idea why it doesn't merge cleanly? Also would you mind filling out our CLA: http://www.discourse.org/cla

@SamSaffron this approach keeps the SQL but isolates it from the controller. I vastly prefer it but would be more comfortable with your approval before merging.

@blowmage

This comment has been minimized.

Show comment
Hide comment
@blowmage

blowmage Feb 7, 2013

Contributor

@eviltrout I created the branch around nine hours ago. If it doesn't merge then you guys must have been busy in that time. :) I'll update on master and re-push.

Contributor

blowmage commented Feb 7, 2013

@eviltrout I created the branch around nine hours ago. If it doesn't merge then you guys must have been busy in that time. :) I'll update on master and re-push.

@blowmage

This comment has been minimized.

Show comment
Hide comment
@blowmage

blowmage Feb 7, 2013

Contributor

FWIW, an accompanying blog explaining the thought behind this change here.

Contributor

blowmage commented Feb 7, 2013

FWIW, an accompanying blog explaining the thought behind this change here.

@eviltrout

This comment has been minimized.

Show comment
Hide comment
@eviltrout

eviltrout Feb 7, 2013

Member

Just wanted to say thanks for an awesome blog post explaining why you did it. It's one thing to link to some code on twitter, it's another to submit a patch to fix it and a detailed blog post of why. I am super impressed, and will happily merge this once @SamSaffron has a chance to review.

Member

eviltrout commented Feb 7, 2013

Just wanted to say thanks for an awesome blog post explaining why you did it. It's one thing to link to some code on twitter, it's another to submit a patch to fix it and a detailed blog post of why. I am super impressed, and will happily merge this once @SamSaffron has a chance to review.

@ZogStriP

This comment has been minimized.

Show comment
Hide comment
@ZogStriP

ZogStriP Feb 7, 2013

Member

👍 for the awesome blog post!

Member

ZogStriP commented Feb 7, 2013

👍 for the awesome blog post!

@nlalonde

This comment has been minimized.

Show comment
Hide comment
@nlalonde

nlalonde Feb 7, 2013

Member

Great changes, thanks so much, blowmage! One minor comment is that UserSearch is a service class, not really a model. It belongs in lib, imho.

Member

nlalonde commented Feb 7, 2013

Great changes, thanks so much, blowmage! One minor comment is that UserSearch is a service class, not really a model. It belongs in lib, imho.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Feb 7, 2013

Contributor

One minor comment is that UserSearch is a service class, not really a model. It belongs in lib, imho.

It models searching for a User. Sounds like a model to me.

(Regardless, I think service classes belong in app/models too: like Mike said in his blog post, lib is for things that could be used on other projects.)

Contributor

steveklabnik commented Feb 7, 2013

One minor comment is that UserSearch is a service class, not really a model. It belongs in lib, imho.

It models searching for a User. Sounds like a model to me.

(Regardless, I think service classes belong in app/models too: like Mike said in his blog post, lib is for things that could be used on other projects.)

@blowmage

This comment has been minimized.

Show comment
Hide comment
@blowmage

blowmage Feb 7, 2013

Contributor

A word on UserSearch. The reason it is a class and not a module, even though it is never instantiated, is because I expected to instantiate it as part of this refactor. Ideally, the UserSearch.search implementation would have instantiated a UserSearch object and looked like so:

def self.search term, topic_id = nil
  UserSearch.new(term, topic_id).search_with_sql
end

That would allow me to add new capabilities in a non-destructive way. So if a pure arel search was added, it could look like the following:

def self.search term, topic_id = nil
  UserSearch.new(term, topic_id).search_with_arel
end

That said, I got sleepy and never got that far. I'm still not sure if an arel/ActiveRecord::Relation implementation is wanted, so I figured I'd submit this anyway and see what happened. I suppose this info would be better in a commit message than a comment on the pull request, but there you go.

¯_(ツ)_/¯

Contributor

blowmage commented Feb 7, 2013

A word on UserSearch. The reason it is a class and not a module, even though it is never instantiated, is because I expected to instantiate it as part of this refactor. Ideally, the UserSearch.search implementation would have instantiated a UserSearch object and looked like so:

def self.search term, topic_id = nil
  UserSearch.new(term, topic_id).search_with_sql
end

That would allow me to add new capabilities in a non-destructive way. So if a pure arel search was added, it could look like the following:

def self.search term, topic_id = nil
  UserSearch.new(term, topic_id).search_with_arel
end

That said, I got sleepy and never got that far. I'm still not sure if an arel/ActiveRecord::Relation implementation is wanted, so I figured I'd submit this anyway and see what happened. I suppose this info would be better in a commit message than a comment on the pull request, but there you go.

¯_(ツ)_/¯

@bbonamin

This comment has been minimized.

Show comment
Hide comment
@bbonamin

bbonamin Feb 7, 2013

Contributor

@blowmage seems like a good choice. I was just about to ask why you decided for a class instead of a module. Your last comment reminded me of this codeclimate blog post http://blog.codeclimate.com/blog/2012/11/14/why-ruby-class-methods-resist-refactoring/

Contributor

bbonamin commented Feb 7, 2013

@blowmage seems like a good choice. I was just about to ask why you decided for a class instead of a module. Your last comment reminded me of this codeclimate blog post http://blog.codeclimate.com/blog/2012/11/14/why-ruby-class-methods-resist-refactoring/

@SamSaffron

This comment has been minimized.

Show comment
Hide comment
@SamSaffron

SamSaffron Feb 7, 2013

Member

I am totally fine with this, more test, more readable code, big wins for Discourse

Member

SamSaffron commented Feb 7, 2013

I am totally fine with this, more test, more readable code, big wins for Discourse

@eviltrout

This comment has been minimized.

Show comment
Hide comment
@eviltrout

eviltrout Feb 7, 2013

Member

❤️❤️❤️

Member

eviltrout commented Feb 7, 2013

❤️❤️❤️

eviltrout added a commit that referenced this pull request Feb 7, 2013

Merge pull request #57 from blowmage/user_search_refactor
Refactor UserController#search_users

@eviltrout eviltrout merged commit 63c0fdd into discourse:master Feb 7, 2013

rustemginiyatullin added a commit to fs/discourse that referenced this pull request Feb 9, 2015

Merge pull request #57 from Continuous-Security/add-title-to-toc-view
[#87059006] Reformat TOC to match updated wireframe

nnrd added a commit to NickIvanter/discourse that referenced this pull request Apr 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment