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

SteamTinkerLaunch: Type hinting and refactoring improvements #448

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sonic2kk
Copy link
Contributor

(Note: This PR is opened as a draft because I have not yet had a chance to extensively test the changes.)

I've been meaning to take a look at the SteamTinkerLaunch ctmod for a while, as it could be brought up-to-date with the other ctmods to use the new utility functions like the fetching project data functions and even the new download_file utility function. Also, the ctmod was written a while ago and not very many changes have been made, and I think there are some logic improvements that could be made (for example, get_tool is huge and could really be split out into separate methods). However before we get that far there are some higher level improvements that I wanted to make, mainly around better type hinting and refactoring to make the ctmod a bit nicer to work with.

This PR adds various type hints around the ctmod (this will help when other functions and methods are more thoroughly type-hinted) and does a bit of refactoring to reduce indentation and make code paths cleaner. Namely:

  • Return much earlier from is_system_compatible if we're on SteamOS, as we don't check dependencies at all on SteamOS.
  • De-indent a lot in get_tool from the tarfile context manager, as we don't need to be indented so much in that block.
    • In future we may even be able to forego the tarfile.open entirely in favour of our helper methods to handle extraction, but I didn't do that in this PR
  • Use continue in at least one loop to continue if a condition is met instead of keeping the logic in an indented block.

Generally in this PR I only made surface-level comfort changes to make the ctmod a bit less "scary". In my head I have a mental image of this ctmod being a bit on the "unclean" side and getting back into this part of the code, it isn't nearly as much of a disaster as I pictured it in my head, but I see a lot of potential for improvement and I want to split it across multiple PRs sequentially. I want to get some basic cleanup in place with this PR, and then move onto splitting parts of the code up a bit better, and then moving onto using our helper methods such as `download_file. All of these steps should help "modernize" this ctmod and make it a lot less "dated". I know a lot more than I did 2 years ago when I initially stepped up to work on this and want to give back by bringing this ctmod up to speed with the other improvements made in the codebase since that time.

If you have any other suggestions for changes I should include in this PR, please let me know and I'll be happy to include them! But again this PR is mainly for those "higher level" changes, like removing unnecessary indentation, returning early, and improving readibility. I understand bigger changes are probably desired but I didn't want to make some monster PR. I think this PR might already be a bit difficult to review with the number of changes despite my best efforts to keep it on the smaller side 😅

All feedback is welcome on this. Thanks!

@sonic2kk sonic2kk marked this pull request as draft August 20, 2024 18:45
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.

1 participant