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 '--all' flag to 'fetch' command #336

Merged
merged 3 commits into from
Jul 15, 2016

Conversation

neslom
Copy link
Contributor

@neslom neslom commented Jul 1, 2016

Fetch all exercises for a given track

As mentioned in issue #335

Fetch all exercises for a given track
@neslom
Copy link
Contributor Author

neslom commented Jul 1, 2016

Hey @kytrinyx! I was looking through the issues and thought I would take a quick stab at this. I'll be offline until Tuesday, but let me know what you think!

@@ -18,7 +18,18 @@ func Fetch(ctx *cli.Context) error {
}
client := api.NewClient(c)

problems, err := client.Fetch(ctx.Args())
args := ctx.Args()
problems, err := client.Fetch(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't perform this fetch if the all flag is set to true. What if we moved it to the else of the following if?

@neslom
Copy link
Contributor Author

neslom commented Jul 2, 2016

Sounds good, @Tonkpils. I'm out of town for the holiday weekend, but will follow up on this when I get back Monday or Tuesday.

@neslom neslom force-pushed the fetch-all-exercises branch from a2ebe3b to c801e2b Compare July 5, 2016 21:00
@neslom
Copy link
Contributor Author

neslom commented Jul 5, 2016

Hey @Tonkpils, I updated the PR with your suggestions. I started to add a test for FetchAll, but wanted to get your input on this before moving forward. FetchAll is essentially just calling List and then Fetch for each item returned by List, so I was unsure if we needed to add a test for this. Also, it looks like the fixtures for tracks and problems would need to match for this test to work.

@Tonkpils
Copy link
Contributor

Tonkpils commented Jul 6, 2016

@neslom I think having tests would be great but if it's too difficult then it's ok for now. The code seems fine. Once we iterate over this and have an actual API then we can add the fixture and simulate that request for the test.

Ensure client.List is called and that client.Fetch is called for each
problem returned by List
@neslom
Copy link
Contributor Author

neslom commented Jul 15, 2016

hey @Tonkpils, I ended up coming up with a test for this! let me know if I did something weird (I'm quite new to Go). I might have gone overboard with the assertions, but I explained my reasoning in the commit message

@Tonkpils
Copy link
Contributor

That looks great! Thank you so much for your contribution!

@Tonkpils Tonkpils merged commit 3be27a1 into exercism:master Jul 15, 2016
@neslom neslom deleted the fetch-all-exercises branch July 15, 2016 18:41
@kotp
Copy link
Member

kotp commented Aug 1, 2016

When does this become available on the client (via exercism upgrade hopefully)?

@Tonkpils
Copy link
Contributor

Tonkpils commented Aug 1, 2016

@kotp we'd need to cut a release out. I'll get to that tonight

@kytrinyx
Copy link
Member

kytrinyx commented Aug 1, 2016

@Tonkpils if you run into any trouble holler—I've tried my best to document the release process, but if I've missed anything let me know and we'll improve the documentation along the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants