-
-
Notifications
You must be signed in to change notification settings - Fork 361
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 list command #1113
Add list command #1113
Conversation
Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed. That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
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 your work on this. I've reopened the PR :)
I have three general comments:
- I really like the list functionality. I'm 👍 on adding that.
- I don't love the download all functionality, as that has the downside of marking all those exercises in progress. I'd like to have a wider community discussion about that first, and also take time to consider it from a product perspective.
- There are quite a few other changes in this PR that make it hard to see exactly what's going on where. I've added questions to those for clarification.
Action suggestions:
- I think there are three seperate PRs here.
a) The refactorings
b) The addition of List
c) The addition of Download
I'd like to see a PR just for (b) and potentially one for (a) to start with. Those will be more manageable to review. In its current state, there's so much happening in this PR, I think it will take a long time to get a proper review. I'd like to get the List stuff reviewed properly instead and be ale to deploy that, without waiting on the rest to get reviewesd.
The List functionality also needs a comprehensive test adding.
Thanks for your work on this. I hope that makes sense!
@@ -116,7 +124,7 @@ func runConfigure(configuration config.Config, flags *pflag.FlagSet) error { | |||
|
|||
// Verify that the token is valid. | |||
if !skipVerification { | |||
client, err := api.NewClient(token, baseURL) | |||
client, err := api.NewClient("", baseURL) |
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.
What does this do?
assert.Regexp(t, "Welcome to Exercism", err.Error()) | ||
// It uses the default base API url to infer the host | ||
assert.Regexp(t, "exercism.io/my/settings", err.Error()) | ||
assert.Regexp(t, "there is no token configured", err.Error()) |
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.
Why does this change?
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 and all the other changes you mention below is a poor attempt at "fixing" the broken tests on master
. Additionally, I need to revert some of the changes that fix deprecated code, mostly from the io/ioutil
to io
and os
packages.
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.
As far as I can see, all tests on main
are green: https://github.com/exercism/cli/actions/runs/6098825660/job/16549554194
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.
According to CONTRIBUTING.MD you can run tests locally with go test ./...
and this fails on a clean clone and master
. I get this:
pwd && go test ./...
/Users/baduker/Downloads/cli
? github.com/exercism/cli/browser [no test files]
? github.com/exercism/cli/exercism [no test files]
ok github.com/exercism/cli/api 0.254s
ok github.com/exercism/cli/cli 0.315s
--- FAIL: TestSubmitFiles (0.01s)
Error Trace: submit_test.go:275
Error: Not equal: "This is file 2." (expected)
!= "" (actual)
--- FAIL: TestSubmitFilesForTeamExercise (0.00s)
Error Trace: submit_test.go:471
Error: Not equal: "This is file 2." (expected)
!= "" (actual)
FAIL
FAIL github.com/exercism/cli/cmd 0.529s
ok github.com/exercism/cli/config 0.370s
ok github.com/exercism/cli/debug 0.424s
ok github.com/exercism/cli/workspace (cached)
FAIL
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.
Do you mean master
or main
? The primary branch on this repo is main
.
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.
Yes, I meant main
, not master
.
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.
Hmmm, I don't know why that's not working on your machine when it is working on our CI. Maybe a different version of Go? CCing @junedev, @andrerfcsantos, @kytrinyx and @ErikSchierboom to see if they have any ideas :)
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.
@iHiD @ErikSchierboom the failing tests happen when using a version of Go 1.17 and above. There was a change to the mime/multipart pkg that changes the file path information being returned for any files included in the mocked request. It looks like there's an open PR for this issue #1066 that can be merged in.
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've replied there. I'd appreciate eyes on that too pls :)
@@ -40,7 +38,7 @@ func TestDownloadWithoutWorkspace(t *testing.T) { | |||
|
|||
err := runDownload(cfg, pflag.NewFlagSet("fake", pflag.PanicOnError), []string{}) | |||
if assert.Error(t, err) { | |||
assert.Regexp(t, "re-run the configure", err.Error()) | |||
assert.Regexp(t, "need an --exercise name or a solution --uuid", err.Error()) |
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.
Why does this change?
@@ -54,7 +52,7 @@ func TestDownloadWithoutBaseURL(t *testing.T) { | |||
|
|||
err := runDownload(cfg, pflag.NewFlagSet("fake", pflag.PanicOnError), []string{}) | |||
if assert.Error(t, err) { | |||
assert.Regexp(t, "re-run the configure", err.Error()) | |||
assert.Regexp(t, "need an --exercise name or a solution --uuid", err.Error()) |
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.
Why does this change?
path: tc.file, | ||
baseURL: "http://www.example.com/", | ||
path: tc.file, | ||
sourceURL: "http://www.example.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.
Why does this change?
|
||
if url != tc.expectedURL { | ||
t.Fatalf("Expected URL '%s', got '%s'", tc.expectedURL, url) | ||
} |
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.
Why remove this block?
I'm closing the PR as this is going to be broken down into two (or more) PRs, starting with this one here. |
List all Exercism tracks, exercises implementation, and download selected exercises.
What does this PR do?
The PR implements a feature in the exercism-cli for listing all the available Exercism tracks and exercises based on the given track and difficulty level. It also allows the users to download the list of exercises for a specific track and difficulty level.
Changes Made:
list
, to the CLI. This command can take the following flags--track
,--all
,--level
, and--download
.downlaod.go
command for readabilityMakeRequest
toapi.go
Related Issue:
How to test it?
add-list-command
go build exercism/main.go
./main configure --token=YOUR_TOKEN_VALUE
Start testing the new command.
Note: the results will be different because the CLI shows tracks and exercises available to a given user.
List your tracks: Run
./main list
This should return:
Note: To see all tracks, run
./main list --all
.List all exercises for a given track: Run
./main --list --track=awk
This should return:
Filter exercises for a given track using difficulty: Run
./main list --track=awk --level=hard
.This should output:
Download selected exercises for a given track: Run
./main list --track=awk --level=hard --download
.This should output: