refactor: add type hints and comments #205

ctrlaltf24 · 2024-10-23T20:51:23Z

Small handful of comments on things I noticed while reading

No functional changes

Small handfull of comments on things I noticed while reading No functional changes

thw26 · 2024-10-24T11:53:28Z

ou_dedetai/config.py

+ACTION: str = 'app'
+APPIMAGE_FILE_PATH: Optional[str] = None
+authenticated = False # FIXME: no references
+BADPACKAGES: Optional[str] = None # FIXME: no references


This is leftover from when we were still using this for removing AppImageLauncher. We ended up having to handle AppImageLauncher differently, but we kept this in case we ever discovered a problematic package.

thw26 · 2024-10-24T11:55:14Z

ou_dedetai/config.py

-PACKAGES = None
-PASSIVE = None
+LOGOS10_RELEASES = None     # used to save downloaded releases list # FIXME: not set #noqa: E501
+MYDOWNLOADS: Optional[str] = None # FIXME: Should this use the tempfile module?


The main reason this exists is for network.reuse_download. Our goal is to minimize network bandwidth for Christian workers in low bandwidth/data limited scenarios.

Ah. Perhaps a separate directory? ~/.cache/FaithLife-Community/...?

thw26 · 2024-10-24T11:56:54Z

ou_dedetai/config.py

-BADPACKAGES = None
+ACTION: str = 'app'
+APPIMAGE_FILE_PATH: Optional[str] = None
+authenticated = False # FIXME: no references


This was from a time when I had implemented password requests using the system keyring, authenticated meaning user password authenticated. At the time we chose to drop this functionality for the sake of user trust and we opted for using pkexec in most situations.

thw26 · 2024-10-24T11:57:41Z

ou_dedetai/config.py

-INSTALL_STEP = 0
-INSTALL_STEPS_COUNT = 0
+FLPRODUCTi: Optional[str] = None
+GUI = None # FIXME: no references


This most likely can be safely removed. I think it would have been used to keep track of the GUI object, but we have the app var we pass around now.

thw26 · 2024-10-24T11:58:00Z

ou_dedetai/config.py

-LOGOS_LATEST_VERSION_URL = None
-LOGOS9_RELEASES = None      # used to save downloaded releases list
+LOGOS_LATEST_VERSION_URL: Optional[str] = None
+LOGOS9_RELEASES = None      # used to save downloaded releases list # FIXME: not set #noqa: E501


thw26 · 2024-10-24T11:58:05Z

ou_dedetai/config.py

-PACKAGE_MANAGER_COMMAND_QUERY = None
-PACKAGES = None
-PASSIVE = None
+LOGOS10_RELEASES = None     # used to save downloaded releases list # FIXME: not set #noqa: E501


thw26 · 2024-10-24T11:59:39Z

ou_dedetai/control.py

@@ -38,6 +38,7 @@ def restore(app=None):
    backup_and_restore(mode='restore', app=app)


+# FIXME: consider moving this into it's own file/module.


I think utils is the place this probably should be in the current file structure.

Its existence here is a holdover from the bash script.

This functionality I think could easily be in its own module, or perhaps we could refactor things to have our various copy functions or otherwise all in a files.py or something similar.

backup.py makes sense to me, there's a lot of logic here

thw26 · 2024-10-24T12:00:13Z

ou_dedetai/control.py

@@ -268,8 +269,8 @@ def set_winetricks():
            # Check if local winetricks version is up-to-date; if so, offer to
            # use it or to download; else, download it.
            local_winetricks_version = subprocess.check_output(["winetricks", "--version"]).split()[0]  # noqa: E501
-            if str(local_winetricks_version) >= "20220411":
-                if config.DIALOG == 'tk':
+            if str(local_winetricks_version) >= "20220411": #FIXME: consider using config.WINETRICKS_VERSION and != string comparision on versions is dodgy #noqa: E501


Sounds like a good improvement.

thw26 · 2024-10-24T12:00:30Z

ou_dedetai/control.py

-            if str(local_winetricks_version) >= "20220411":
-                if config.DIALOG == 'tk':
+            if str(local_winetricks_version) >= "20220411": #FIXME: consider using config.WINETRICKS_VERSION and != string comparision on versions is dodgy #noqa: E501
+                if config.DIALOG == 'tk': #FIXME: CLI client not considered


See #170, which may affect this

thw26 · 2024-10-24T12:01:32Z

ou_dedetai/control.py

@@ -296,6 +297,7 @@ def set_winetricks():
                        )
                        return 0
                    else:
+                        # FIXME: Should this call a function on the app object?


The simple answer is we haven't spent much time reviewing control.py for a while. It probably needs reviewed, as do some other aspects of code. See also our comments in #35 and strewn about the code.

thw26 · 2024-10-24T12:01:56Z

ou_dedetai/control.py

@@ -319,7 +321,7 @@ def set_winetricks():
    return 0


-def download_winetricks():
+def download_winetricks(): #FIXME: unused, logic moved to system.install_winetricks


Good catch. I'm sure this was left behind due to a bad rebase.

thw26 · 2024-10-24T12:02:16Z

ou_dedetai/installer.py

@@ -677,6 +677,7 @@ def ensure_launcher_shortcuts(app=None):
    ensure_launcher_executable(app=app)
    config.INSTALL_STEP += 1
    runmode = system.get_runmode()
+    # FIXME: why not do this all the time?


Good idea. Should be easy to move to main

thw26 · 2024-10-24T12:02:52Z

ou_dedetai/system.py

@@ -259,7 +260,7 @@ def reboot():
    sys.exit(0)


-def t(command):
+def t(command): # FIXME: unused, have_dep does the same thing


I've noticed this before but didn't give it much thought. Good to replace one with another.

thw26 · 2024-10-24T12:04:00Z

ou_dedetai/system.py

@@ -275,6 +276,7 @@ def tl(library):


 def get_dialog():
+    # FIXME: wouldn't the cli still work without a DISPLAY? What about wayland?


This is about when executing the wine functions that require a GUI, not necessarily our CLI. We'd need to make this specified based on the CLI command run.

thw26 · 2024-10-24T12:04:16Z

ou_dedetai/utils.py

@@ -225,7 +225,7 @@ def delete_symlink(symlink_path):
            logging.error(f"Error removing symlink: {e}")


-def check_dependencies(app=None):
+def check_dependencies(app=None): # FIXME: misnomer, isn't this an install_dependencies


Holdover from a refactor

thw26 · 2024-10-24T12:04:55Z

ou_dedetai/utils.py

@@ -442,7 +442,7 @@ def get_winetricks_options():
        # Check if local winetricks version is up-to-date.
        cmd = ["winetricks", "--version"]
        local_winetricks_version = subprocess.check_output(cmd).split()[0]
-        if str(local_winetricks_version) >= "20220411":
+        if str(local_winetricks_version) >= "20220411": #FIXME: consider using config.WINETRICKS_VERSION and != string comparision on versions is dodgy #noqa: E501


Sounds like a good improvement.

thw26

Reviewed your comments. A lot sound like good TODOs/FIXMEs. Left my thoughts on others that might need more work.

refactor: add type hints and comments

0faaf9c

Small handfull of comments on things I noticed while reading No functional changes

thw26 reviewed Oct 24, 2024

View reviewed changes

thw26 merged commit 1770c8d into FaithLife-Community:main Oct 24, 2024

ctrlaltf24 deleted the refactor-add-type-hints branch October 24, 2024 22:02

ctrlaltf24 mentioned this pull request Oct 24, 2024

refactor: handfull of fixmes resolved trivally #212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add type hints and comments #205

refactor: add type hints and comments #205

ctrlaltf24 commented Oct 23, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024 •

edited

Loading

ctrlaltf24 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

ctrlaltf24 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 Oct 24, 2024

thw26 left a comment

		@@ -38,6 +38,7 @@ def restore(app=None):
		backup_and_restore(mode='restore', app=app)


		# FIXME: consider moving this into it's own file/module.

		@@ -275,6 +276,7 @@ def tl(library):


		def get_dialog():
		# FIXME: wouldn't the cli still work without a DISPLAY? What about wayland?

refactor: add type hints and comments #205

refactor: add type hints and comments #205

Conversation

ctrlaltf24 commented Oct 23, 2024

Choose a reason for hiding this comment

thw26 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thw26 left a comment

Choose a reason for hiding this comment

thw26 Oct 24, 2024 •

edited

Loading