Skip to content

Display problems that have not yet been submitted#155

Merged
Tonkpils merged 1 commit intoexercism:masterfrom
Tonkpils:fetch-output
Jan 30, 2015
Merged

Display problems that have not yet been submitted#155
Tonkpils merged 1 commit intoexercism:masterfrom
Tonkpils:fetch-output

Conversation

@Tonkpils
Copy link
Contributor

We're pretty much querying the exercises API from exercism.io to get a list of submitted problems. Since we only want to check for problems that are stopping the user from fetching the next exercise we'll display the ones that have not been submitted.

@Tonkpils
Copy link
Contributor Author

I ended up using the same report format for this PR. I found it a bit more intuitive but if needed we can change it to explicitly state that problems not yet submitted will not allow continuing on the track.

This is a sample of the output.

% out/exercism fetch go

 Not Submitted: 1 problem
Gigasecond (go) /Users/tonkpils/exercism/go/gigasecond

unchanged: 3, updated: 0, new: 0

cmd/fetch.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR looks great! What do you think about passing the map[string][]SubmissionInfo in to this function ? I think that would make it easier to test.

Copy link
Member

Choose a reason for hiding this comment

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

That would probably be nice. Another thing we could do is have an alias type for []*api.Problem that has a function to update the state. It could both make the call and do the loop. Not sure what that would look like in terms of testing, though.

@kytrinyx
Copy link
Member

I ended up using the same report format for this PR.
I like the report format being consistent -- I think that will make it easier for people to parse things.

if needed we can change it to explicitly state that problems not yet submitted will not allow continuing on the track.

Yeah, I think that would be good. Another thing that people can do is to call list and then explicitly get another problem, so they're not completely blocked, but they won't automatically get any more problems until they submit the unsubmitted one(s).

@Tonkpils
Copy link
Contributor Author

One side effect that I just found is that if we call hw.Summarize() and the problems list has not had their submission state set, like its being done on the cmd.Fetch, then it will display them all as not submitted. I'm wondering if this should be done at the api.Fetch level. What do you think?

@lcowell
Copy link
Contributor

lcowell commented Jan 21, 2015

I had the same thought - to have Fetch also grab the submission information. I like it because it makes it harder to get things wrong.

Only a call to cmd.Demo calls api.Fetch, but doesn't require the submission information. What do you think about having a couple of different api methods. One that calls fetch and grabs the submission info and one that doesn't. Maybe api.Fetch and api.FetchDemo ?

The other idea I had was to extend the xapi response to also include this information in a single call when appropriate. I have no idea how much work is involved in that.

@kytrinyx
Copy link
Member

Yeah, I think you're right that Fetch should get it. Also I think it's wise to have two different methods if we see that behavior is diverging.

@Tonkpils
Copy link
Contributor Author

Yeah, I was thinking about that yesterday. Ideally the Fetch api would not be used by anyone else other than the Fetch command. However, the way it was written, it was flexible enough to allow this. I agree to have different API methods would help this. I can add the submission state check to Fetch. Do you want me to also separate the methods in this PR or should that be done separate?

Update
So, adding the submission check into Fetch would actually require to change the function to take more arguments (either the config or the API Key and host).

Would you mind if I take a stab at modifying the api package to use a Client struct that contains the config and reuses the http.DefaultClient? That way it'd be easier to move forward with this change.

@kytrinyx
Copy link
Member

Would you mind if I take a stab at modifying the api package to use a Client struct that contains the config and reuses the http.DefaultClient

Not at all, I was just thinking about this the other day. I think it's the right choice.

@lcowell
Copy link
Contributor

lcowell commented Jan 21, 2015

There are a couple of places we don't set the user agent constantly. It would be amazing if introducing our own client hid that detail.

@kytrinyx
Copy link
Member

There are a couple of places we don't set the user agent constantly. It would be amazing if introducing our own client hid that detail.

Totally!

@Tonkpils Tonkpils force-pushed the fetch-output branch 2 times, most recently from d7f6712 to 79ec879 Compare January 23, 2015 21:49
@kytrinyx
Copy link
Member

@Tonkpils is this one ready?

cmd/restore.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a custom type and a constant flag to make this more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of making a struct to take options on what to summarize but decided it wasn't worth the complexity if we didn't need the functionality.

Can you give me an example of what you mean? I definitely do agree that it could be more readable.

@Tonkpils
Copy link
Contributor Author

Once I address the readability comment it'll be ready :)

@kytrinyx
Copy link
Member

Cool, ping me when you're done. I sometimes lose track of things without new notifications.

@Tonkpils
Copy link
Contributor Author

@kytrinyx What do you think of the last commit? If that is ok with you then I'll go ahead and squash the commits and merge.

@kytrinyx
Copy link
Member

I don't think that a struct is the best choice here, since we will only ever use one of the three fields at any one time.

Take a look at the HWFilter type. Could we use that or something similar?

@Tonkpils
Copy link
Contributor Author

Got it, I think that should do it. Wasn't too sure on the naming though.

@kytrinyx
Copy link
Member

Yeah I think this works. When you say that you're not so sure about the naming, do you mean the HW prefix? Perhaps we can change the prefix to Filter, which is more generic.

Either way, I think this is ready for squashing and merging.

@Tonkpils
Copy link
Contributor Author

Nah, I meant more the summaryFilter and SummaryOption. Those should be easy enough to tackle after they are merged though.

@Tonkpils Tonkpils force-pushed the fetch-output branch 3 times, most recently from 69ed8da to 7d28961 Compare January 30, 2015 05:18
Tonkpils added a commit that referenced this pull request Jan 30, 2015
Display problems that have not yet been submitted
@Tonkpils Tonkpils merged commit a6ac2c8 into exercism:master Jan 30, 2015
@Tonkpils Tonkpils deleted the fetch-output branch January 30, 2015 05:23
@kytrinyx
Copy link
Member

Yeah, I think it's good. Thanks for this!

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.

3 participants