-
Notifications
You must be signed in to change notification settings - Fork 22
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
Adding filter by status flag to Challenges #20
Adding filter by status flag to Challenges #20
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.
Great contribution! Thank you for your effort! I'll be happy to merge it - let's just clean it up a tiny bit.
internal/api/challenges.go
Outdated
} | ||
|
||
func (c *Client) ListChallenges(ctx context.Context, opts *ListChallengesOptions) ([]*Challenge, error) { | ||
var challenges []*Challenge | ||
query := url.Values{} | ||
if opts.Category != "" { | ||
query.Set("category", opts.Category) | ||
if opts.Category != nil { |
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.
It's possible to iterate over a nil
slice in Go - the if
check is redundant.
&opts.category, | ||
"category", | ||
"", | ||
`Category to filter by - one of linux, containers, kubernetes, ... (an empty string means all)`, | ||
[]string{}, |
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.
The an empty string means all
part is probably a leftover from the previous implementation.
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.
Done
cmd/challenge/catalog.go
Outdated
&opts.status, | ||
"status", | ||
[]string{}, | ||
`status to filter by - one or multiple status like todo, attempted, solved ... (empty means all))`, |
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.
Same as above - probably no need to add empty means all
Is there a typo in the PR description in available statuses?
and in the example you mention status 'solved'
I also wonder if you should return all challenges if you provide an invalid status vs none. |
Thanks @jedrekdomanski for reviewing |
I may have misexpressed myself. What I wanted to say was that I feel like it doesn't make sense to return all challenges in case of an invalid status, which is happening now. What about we raise an error if an invalid status is supplied to avoid confusion? Right now you return all challenges even if you provide an invalid status which might be misleading. |
@jedrekdomanski This is what i see if an invalid status is provided. Returning empty on invalid seems like the behaviour of the rest api.
|
Ah i see whats happening, its returning when labctl isnt logged in! |
d9e9856
to
0099902
Compare
Looks great! Thank you, @wololowarrior, for your time and effort! |
Issues
status
, like if it was attempted or solved by the user or still a todo.What does this PR do?
--status
flag tolabctl challenge catalog
, values aretodo
,attempted
,solved
More
category
Examples