-
Notifications
You must be signed in to change notification settings - Fork 21
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 list to accept filter and match on class name #39
Allow list to accept filter and match on class name #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top work Brad! 🙌 Just some few suggestions from me, lmk what you think otherwise good to go!
@@ -1 +1 @@ | |||
2.4.0 | |||
2.5.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bumping ruby! 👍
expect(output.string).to match(/Faker/) | ||
expect(output.string.lines.size).to be_positive | ||
end | ||
|
||
context 'when passing a filter option' do | ||
let(:options) { super().merge(filter: filter) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoah, I didn't know you could call super
on a let
object. Nice! 🎖 😃
context 'when single `list` command' do | ||
it 'executes successfully' do | ||
command.execute(output: output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're warming up to the "mystery guest mitigation" style 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am! Maybe we should talk about standardizing on a spec style across the repo. If you want to standardize on the mystery guest mitigation style, we can remove the ‘let’s and have code that’s duplicated, but easier to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely open to that. I wonder if we can configure rubocop-rspec to enforce those rules. 🤔 But I really don't have any strong feelings on this one. 🙁
|
||
private | ||
|
||
def render(result, output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice specialisation here. Eventually, I think we might define the Command#render
superclass to have this shape since we have the same on the search command. But I think this is great for now as we might add some new commands (don't know which ones yet) and they might have a different shape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s true, good call. I’ll keep that in mind moving forward!
@@ -20,6 +20,44 @@ | |||
expect(sample_result_value).to be_a(Array) | |||
expect(sample_result_value).not_to be_empty | |||
end | |||
|
|||
context 'when passing a filter' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️
Co-Authored-By: Kabiru Mwenja <makabby@gmail.com>
Co-Authored-By: Kabiru Mwenja <makabby@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses the list features requested in #20
New features:
list
accepts a filter, which can be a full class name or partial (case-sensitive) match. If there is no match, we return a 'not found' messageCloses #20