CanastaWiki / Canasta-CLI

The Canasta command line interface, written in Go
MIT License
5 stars 14 forks source link

Added option to choose version to install #77

Closed chl178 closed 9 months ago

chl178 commented 1 year ago

fixes #34 enhancement for #35

Summary

This pull request enhances the install.sh script for the Canasta CLI installer by introducing improved error handling, better readability, and new features such as argument parsing, version selection, interactive mode, and the ability to list available versions. The changes aim to provide a more comprehensive and user-friendly installation experience.

Key changes:

List available versions: Implemented the list_versions function, which fetches and displays a list of available Canasta CLI versions from GitHub releases. Interactive mode: Added support for interactive mode, allowing users to choose the desired version and confirm other installation options interactively. Version selection: Improved the choose_version function to handle version selection more effectively, with better input validation and support for interactive mode. Dependency checking: Consolidated dependency checking in the check_dependencies function, making it easier to manage dependencies and inform users about missing tools. Script organization: Reorganized the script's main flow into the main function, which calls the other functions in the correct order, improving the script's overall structure.

Please let me know if you have any questions or concerns about the changes in this PR.

chl178 commented 1 year ago

@jeffw16 @yaronkoren @amalpaul54111 would you please take a look at this?

yaronkoren commented 1 year ago

@chl178 - sorry for the delay; I just looked through the code. I haven't actually tried running it, but I imagine it works as advertised. Overall it looks great. I do have some questions:

chl178 commented 1 year ago

Thank you for your suggestions.

  • What is the benefit of "interactive mode"? It seems strange for a user to specify that they want to enter a version number, but not just yet. Maybe interactive mode should just be the default, i.e. if the user doesn't specify a version, the script asks them for one (and then if they leave it blank, it goes with the latest)?

You're right, the "interactive mode" isn't necessary in this specific case. We should set it as the default.

  • The previous code checked for the existence of the wget "--show-progress" flag; this code does not. Is there a reason for that?

Thank you for your reminder.

  • Speaking of wget, maybe the dependencies check should go right at the beginning? That way, you also won't need to check twice for the existence of wget?

I have moved checking function to the beginning.

yaronkoren commented 1 year ago

Fantastic - this looks great now. (And done very quickly!) My only (small) comment is that it doesn't look like there's a reason any more to check for the existence of wget in list_versions().

chl178 commented 1 year ago

Oh, thank you very much for your careful inspection! I have been working on Canasta/#223, I have basically completed the code, and will commit in a few days.

yaronkoren commented 1 year ago

This looks good to me - hopefully someone can test it, so it can be merged in soon!

(And that's great to hear about the other task also.)