Initial refactor for list command

[baduker]

Refactor download.go in Preparation for Add list command

Purpose of this PR:

This PR primarily aims to simplify the download command, paving the way for the seamless integration of the list command. This will eventually allow users to list tracks and related exercises (refer to the linked PR for details).

Key Modifications (ordered for clarity):

1. download.go:

• Introduced a shared *api.Client across all methods and functions for uniformity.
• Decomposed runDownload and newDownload functions to enhance readability.
• Eliminated superfluous usrCfg verifications.
• Extracted usrCfg items from solutionDownload structs, decoupling configuration from download operations.
• Modularized and delegated the setup and validation of the download command flags to distinct methods.
• Unified the creation process of solutionURL.
• Refactored method names, replacing vague identifiers like url(), files().
• Reorganized the structure: placed all structs at the beginning, and logically grouped related methods/functions.
• Augmented all functions, methods, and structs with descriptive comments.

2. client.go:

• Introduced MakeRequest to merge the functionalities of client.NewRequest and client.Do, enhancing the DRY principle.

3. configure.go:

• Implemented LoadUserConfig(), which encapsulates the logic for fetching the user config governed by Viper.

---

Compatibility & Testing:

To the best of my knowledge, this PR introduces no breaking changes. All previously established functionalities should persist undisturbed. Both the go test ./... command on this branch and the test suite in download_test.go have been executed without any discrepancies.

[github-actions]

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.

[iHiD]

@baduker Thanks!
@nywilken Would you be happy to review this pls?

[nywilken]

@baduker Thanks! @nywilken Would you be happy to review this pls?

@iHiD I can take a look this week for sure.

[nywilken]

Hi @baduker thanks for opening up this change. I've started reviewing but won't be able to complete today. I'll pick it up on Monday.

So far I see what you did with the MakeRequest function but I find the function signature a little confusing as it doesn't indicate that providing false to isFullUrl will automatically append the API URL or that the MakeRequest will always make a Get request.

I'll give it a more complete review on Monday. In the mean, time we've updated the minimum Go version to 1.20 could you rebase your changes on to the latest main to get the test passing again. Thanks.

[baduker]

Hi @nywilken! Thanks for looking at this. Fair point about the MakeRequest function. I'll see what I can do to improve the signature and make the isFullUrl more explicit. As for the upgraded to Go 1.20, I've already rebased and force-pushed, however, I don't get the checks executed because the workflow is awaiting approval. Any idea why that is?

Looking forward to your full review and the discussion!

[nywilken]

I've already rebased and force-pushed, however, I don't get the checks executed because the workflow is awaiting approval. Any idea why that is?
Looking forward to your full review and the discussion!

@baduker apologies for the delay. I wasn't able to make time on Monday. I've allocated time for my Friday. Apologies again.

As for the the required checks, I believe their is a repo setting for PRs that I don't have access to change. Maybe this is something that @ErikSchierboom can assist with when he has a moment.

[baduker]

Hi @nywilken! Thanks for keeping me posted and don't worry too much about it. We all do open software in our spare time and sometimes it's just not enough of it.

Anyways, drop me a line when you're done reviewing and I'll get back to you as soon as I can.

[nywilken]

@baduker thanks again for the change and for your understanding with my time to review.

I get the approach but I think we may need to break up the change a bit more to help provide small actionable suggestions. I find a few of the function rewrites to duplicate the data being passed along like client and userCfg in the download command.

But to better assist with the review could you share what parts of the code you think could benefit from a readability rewrite and start there.

For the shared code I can see you wanting to reuse for the list command so that makes sense and maybe starting there will help clean things up a bit to start.

Also I just want to say that my feedback is not a hard stance and we can definitely iterate on things.

I look forward to hearing your thoughts.

[nywilken]

Seeing as cfg is initialized and passed immediately into runDownload, what do you think about calling it from within runDownload?

[baduker]

Well, this is not a bad idea, but if I move LoadUserConfig() into runDownload(), then I'd have to change the download_test.go. Should I go for that change, too?

[baduker]

Hey @nywilken! Thanks for the review and feedback. I haven't really gotten around to it yet but give me a couple days and I'll get back to you some changes and points for discussion.

[baduker]

Hello @nywilken! I've been unwell recently, but I'm back now, full of energy and ready to go. I need a little time to refresh my memory on the changes. I'll get back to you with an update in a few days. Thank you for your patience!

[nywilken]

Hello @nywilken! I've been unwell recently, but I'm back now, full of energy and ready to go. I need a little time to refresh my memory on the changes. I'll get back to you with an update in a few days. Thank you for your patience!

@baduker glad to hear you are well and on the mend ❤️‍🩹. Please take care and ping whenever you’re ready.

Things on the PR front seem to be on hold for a bit as the team’s priorities are focused on the platform. I will provide any guidance I can but I cant merge or fix the required checks issue for Go 1.15.

[baduker]

Hello @nywilken! I trust this message finds you in good health. I've implemented most of your suggestions for the PR and have updated my changes. However, there's one thread that remains unresolved, as I require further input from you. I eagerly await your response!

[baduker]

Hello @nywilken! I hope you're doing well. I wanted to check in with you regarding the status of this pull request. I'm eager to complete this if you still see the need for the list command. Please let me know! Take care!

[baduker]

@nywilken is this thread still alive? @ErikSchierboom this has stalled a bit. Should I drop the changes or are you still up for the enhancement?

[ErikSchierboom]

In a previous comment, @nywilken said:

I get the approach but I think we may need to break up the change a bit more to help provide small actionable suggestions

I agree with this. This PR is quite a big refactoring, which makes it hard to review. I'm not opposed to some refactoring/cleanup, but I'd like them to be as minimal as possible.

I'm not well versed into the CLI code base. Maybe @ekingery could also take a look?

[ekingery]

I'm not well versed into the CLI code base. Maybe @ekingery could also take a look?

I am missing some context. The description says:

This will eventually allow users to list tracks and related exercises (refer to the linked PR for details).

Is there a linked PR? Is this related to a wider initiative that we are planning? If not, while these appear to be worthwhile adjustments, the risk seems high for a non functional change, given we have noone with experience on this codebase to review it.

Perhaps if we had coveralls or something similar and we could show full test coverage on the diff, that would provide more confidence. Alternatively, creating (if it does not exist) and running through a documented procedure for manually verifying related functionality in the CLI might be the next step?

[ErikSchierboom]

@baduker I'm going over all the PRs and whilst browsing this PR, I think my main concern is that it is doing so many different things. Whilst there are definitely things in this PR worth exploring, I'm gonna close this PR.

If you'd still like to work on this codebase, please open an issue on the forum first: https://forum.exercism.org/c/exercism/4 We can then discuss what would be worthwhile to work on.