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

Allow search to accept namespaces and partial matches #38

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

bpleslie
Copy link
Contributor

@bpleslie bpleslie commented Jul 25, 2019

This PR addresses the search features requested in #36.

New features:

  • search by namespace supports the full class name, namespace, lowercase namespace and partial namespace
    Screen Shot 2019-07-25 at 2 25 58 PM
  • search by method name supports full method searches when the method includes _ and partial searches
    Screen Shot 2019-07-25 at 2 28 11 PM
  • we also avoid the unintentional Faker::Kpop i_groups match when searching simpsons
    Screen Shot 2019-07-25 at 2 35 50 PM

If the class name matches, we'll return all of the class methods.
If the class name does not match, we'll check to see if any of the methods match.

Let me know any feedback, especially on the spec style 😄

@bpleslie
Copy link
Contributor Author

@akabiru @vbrazo tagging you both for visibility!

Copy link
Owner

@akabiru akabiru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! 👍

@@ -31,14 +31,20 @@ def call
def search_descendants_matching_query
faker_descendants.each do |descendant|
methods = descendant.my_singleton_methods
matching = methods.select { |method| query_matches?(method.to_s) }
store(descendant, matching)
if query_matches_class_name?(descendant.to_s)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really love the improvement here, I can already see a lot more results! 🙌

context 'when no match is found' do
it 'returns an empty hash' do
query = 'foobar'
context 'when passing a full class name' do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test examples! 🎖

@@ -4,25 +4,48 @@

RSpec.describe Faker::Bot::Reflectors::Search do
describe '#call' do
context 'when a match is found' do
let(:result) { described_class.new(query).call }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting change here, it's definitely more reusable across the test examples. I've been trying out a different approach by defining verification logic in the test examples to mitigate the Mystery Guest test smell.

To be fair, this is a very common testing pattern in the RSpec world, so I doubt many people will have trouble making sense of it. Happy to hear your thoughts on the same. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great article! I was trying to decide if I should follow the existing pattern or avoid duplicate code, since so much of it could be shared. I'm happy to adopt the testing style you've already implemented in this app moving forward, just let me know!

@akabiru akabiru merged commit a5ebe64 into akabiru:master Jul 26, 2019
@bpleslie bpleslie deleted the feature/36-namespace-searching branch July 26, 2019 11:57
@akabiru akabiru added this to the 0.5.0 Release milestone Jul 31, 2019
@akabiru akabiru mentioned this pull request Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants