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

Removes extra github requests for each tag by reusing data from the List Releases requests #18

Merged
merged 3 commits into from
Dec 17, 2023

Conversation

ddotthomas
Copy link
Contributor

No longer builds a github api request from a release tag. Now using one Github API call to get Download info Cleaned up main.rs; now it parses user data and starts a function Fixed some minor spelling mistakes and other errors

No longer builds a github api request from a release tag.
Now using one Github API call to get Download info
Cleaned up main.rs; now it parses user data and starts a function
Fixed some minor spelling mistakes and other errors
@auyer
Copy link
Owner

auyer commented Oct 25, 2023

Hi. Thanks ! I will take a a few days look at your PR because I am traveling.

@auyer auyer added the 🐛 Bug Something isn't working label Oct 25, 2023
@auyer
Copy link
Owner

auyer commented Dec 9, 2023

Hi, I am only now able to check this PR.
Can you please describe the issues you are trying to fix?
I see you changed a lot of code, and this makes it hard to review this PR.
Also, I could not find an error like you described in the title.

@ddotthomas
Copy link
Contributor Author

ddotthomas commented Dec 9, 2023

First off I moved much of the program out of the main function and into their own respective functions. In doing so I noticed the function github::fetch_data_from_tag is called several times originally in main. I changed the programs logic (which was a lot of code to do, sorry) to only call the github list_releases which already would return all the data from fetch_data_from_tag data but in a large list of each tag/release. I switched to using that one large list which was already in memory/ in the RAM instead of making further web requests to Github.

I can't find the tag/version that caused it now but I think one of the tags from either steam or Lutris would error out saying github couldn't find anything at the requested URL, so I feel that using the data from the one list instead of making further calls also helps the stability of the program and keep potential web/internet errors to a minimum.

I also merged the tag_menu and variants_menu functions in the helper_menus into one multiple_select_menu function which does the same functionality as both by switching the passed in Vec into a Generic Type that implements Display which the AppInstallations and String type both do.

Most of the large deletions and additions are from moving the download logic out of main and into download.rs. If you read download.rs you should recognize the same functionality that was originally in main.

I also tried to implement some functional programming principles, meaning before in main the program would update several bool variables such as 'should_detect_apps' and then run through several if and else statements to parse the user's data. I switched the logic to instead check the user's answer once in the match statement against the 'answer' variable and then run whichever associated function that corresponds or matches that answer. I think this will save the CPU a couple branches when it doesn't have to check if statements that are no longer relevant to what the user is trying to do. For example, if the user wants to open the tag selector, originally the program or CPU would have to check against manage_existing_versions, should_detect_apps and a couple other if statements in order to get to the functionality in the program we actually want in this case. By instead launching the function we want right from the initial match statement we can skip all that and save some resources while achieving the same result.

Let me know if you need me to discuss or explain anything further.

@auyer
Copy link
Owner

auyer commented Dec 10, 2023

Some of these changes I wanted to make too. I agree the main file needed some work.
I will make some tests in your branch now. Thanks!

Copy link
Owner

@auyer auyer left a comment

Choose a reason for hiding this comment

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

I will make one small change to the PR before merging

protonup-rs/src/download.rs Show resolved Hide resolved
@auyer auyer changed the title Fixed selecting Proton version erroring out Removes extra github requests for each tag by reusing data from the List Releases requests Dec 14, 2023
@auyer auyer merged commit d3bf27b into auyer:main Dec 17, 2023
5 checks passed
@auyer auyer added 💅 Improvement Improvement that are not features and removed 🐛 Bug Something isn't working labels Dec 17, 2023
@ddotthomas ddotthomas deleted the fix-custom-proton-version branch February 25, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💅 Improvement Improvement that are not features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants