networkutil: Add new download_file utility function

[sonic2kk]

Overview

This PR adds a new utility function, download_file, which takes the repeated download code in ctmods and turns it into a generic function.

This function lives in a new file, networkutil.py, and in future we can refactor and move the other network-related functions in here. Instead of further polluting util.py I figured I'd make a new file, which should also hopefully reduce the load when refactoring and moving the other methods for fetching from GitHub/GitLab into this file. Doing this should also aid in writing tests!

Background

Each ctmod has to download the respective archive from its API. In almost every ctmods case (or maybe actually for all of them) the code is duplicated and does the same thing:

• Make a request to the API endpoint
• Figure out the filesize
• Figure out how many chunks we need to download in based on the file size and buffer size
  • So if we want to download 65536 bytes at a time, when downloading we only load 65536 bytes into memory at a time when downloading.
  • Since we stream the request, we can do this lazily and overall save on memory.
  • Using chunks also lets us figure out the download progress
• Create the destination path and file to write the bytes of the response content into
• Loop over the response content as per the buffer size (i.e. 65536 bytes at a time)
  • Stop the download if it was cancelled at any point (i.e. ProtonUp-Qt closes)
  • If we have a valid chunk, write it into the file
  • Figure out the download progress and emit it to the progress bar
• Return True if download succeeded, False otherwise

This is a repeated 25 lines or so across each ctmod. Each time we create a new ctmod this is copied and pasted. If we ever had to adjust this we would have to potentially touch quite a few ctmods. A base ctmod class may resolve this somewhat but in the meantime it puts complexity for a generic action inside of a ctmod. There is a better way!

The Better Way

(Okay, "better way" is an exaggeration 😄)

The tl;dr is that we now have a util function that can download files. It is a close copy of each ctmod's __download method but has more catches for handling errors, and also has better logic for grabbing the file size by allowing us to pass a known file size, so we can consistently work out the download progress, even if Content-Length is blank, without needing to resort to len(response.content) which defeats the purpose of streaming the file and stalls the function call. We should have the file size head of time in most cases as when using fetch_project_release_data one of the fields we try to parse from the assets list is the filesize. GitHub and GitLab should have the size in its data, but GitLab is the only one that doesn't have Content-Length in the response headers because it uses Transfer-Encoding: Chunked. So instead of using len(response.content) we can use the known size to work out the download progress. This also means this new util function fixes the currently broken DXVK Async progress bar.

Instead of repeating all of this logic in each ctmod, we can make a util function for this. Downloading files is a very generic action we would generally want to use the same implementation, at least when it comes to ctmods.

So that's what this PR does. I re-wrote the ctmod download function by hand, added in some extra try/except checks, and even did a little extra spice I'll talk about in a second.

The function takes the following parameters:

• Required: The URL to download from, as standard
• Required: The destination path to download to, again as standard
• Required: The progress_callback which is a function we can call from inside our download function to send the download progress
  • For us, this is the __set_download_progress_percent method on each ctmod that handles emitting to the progress bar! This is how we update the progress from inside this function.
• Optional: download_cancelled property, which is a Property class specific to Qt. We use this to manage if the download was cancelled on-the-fly (i.e. if ProtonUp-Qt was closed)
  • I am not so comfortable with this. Property is specific to Qt, which makes this function far less generic than I'd like. The only alternative I found was to use some kind of async signals that Python has, but then that becomes a bit too generic. So I think using Property was the path of least resistance.
  • It is optional at least, since if we used this elsewhere we may not care about cancelling the download.
• Optional: The buffer_size which defaults to the same as what most ctmods use, 65536
• Optional: The stream boolean given to requests.get to know whether to stream the request or not. This defaults to True, which is what the current ctmod download method uses.
  • This is allowed to be defined for a specific reason I'll discuss below
• Optional: The known_size which is the size of the file we want to download, if we know it ahead of time we can pass it in meaning we don't have to rely on Content-Length which may not always be available, and we may not always be able to get the file size (meaning we won't be able to calculate the download progress)
  • This defaults to 0 which means we fall back to trying to figure out the file size ourselves.

A lot of this is pretty standard, but being able to specify stream and known_size might seem strange, so I'll explain. This is the "extra spice" I was talking about!

Getting the File Size to Calculate Download Progress

Currently when we download files, we pretty much always use stream=True. This won't load the whole request to my knowledge, saving on memory. We then get the file size by taking it from the response's Content-Length header. But, this header is not always available. It should always be given when making a request to GitHub, but GitLab does not send it.

The reason GitLab does not send this header is because it uses a Transfer-Encoding type of Chunked. This basically downloads the files in chunks already I guess. But this means we can't get the file size! To work around this in #302, I simply used len(response.content) based on guidance in this StackOverflow answer. But this has two problems:

• This defeats the whole purpose of stream=True as it loads the entire response.
• This stalls the function. As a result, the download progress does not work properly for DXVK Async. The progress bar goes to 1% and then zooms up to 98-99%, and then extracts very quickly.

In other words, right now, if we don't have Content-Type, we can't set the download progress properly.

However, we aren't defeated yet! When we grab the asset we want to download from GitHub or GitLab using fetch_project_release_data we can get back the size from the release asset. This should be available on release API responses for both GitHub and GitLab, but we only need it for GitLab because we have no other way of getting the file size without resorting to len(response.content), which is undesirable for the reasons above.

So when we call our new util function, it will do the following to figure out the filesize:

• If we gave it a known_size above 0, we will use this.
• Otherwise
  • Try to set the file size to the Content-Length header value
  • If we don't have the Content-Length header:
    • Try to use len(response.content) to get the file size, but ONLY if we are NOT streaming the response.
    • This is because if we try to do this we won't even end up reporting the progress anyway, and it would defeat the purpose of passing stream=True.
      • If we know we won't have a known filesize, we should set stream=False when calling the util function.
• If after all this the filesize is still 0, print an error and return False because if we don't have the file size we can't report the download progress, resulting in a broken progress bar (and we are probably using the function wrong in such a scenario).

This is why we can pass whether or not to stream the response, and why we can pass a known file size to the function. If we already have the size when calling the function, there is no need to fetch it in the function, we should prefer our known size. Similarly if we know we won't get Content-Length back in our response headers (such as if we know we're making a GitLab API call without a known file size ahead of time) then we can pass stream=False so that we load the request response upfront when making the call.

And because we can pass known_size to this util function, this means our GitLab calls now have fixed progress bars! DXVK Async is the only GitLab call we make right now, and it has a broken progress bar because we have to use len(response.content). This results in the progress bar going to 1% and then when the download finishes it goes to 99% when the function ends. With this PR, the progress bar works correctly.

The explanation may be a bit long-winded but I hope it helps illustrate what the function does similarly and differently, why I made certain choices, and the benefits of those choices. It basically allows us to set the file size ahead of time if we know it instead of relying on the response which may not always give us the results we want, and for ctmods we should usually have this in the data dict anyway.

Implementation

Now that explanation of how it works is out of the way, here's how the function is implemented in this PR.

I decided to only refactor the __download call in two ctmods for now: DXVK (includes DXVK Async) and Luxtorpeda (includes Boxtron and Roberta). The rationale here is that we modify a lower-risk ctmod for each launcher; DXVK for Lutris and Heroic, Luxtorpeda for Steam.

Modifying these ctmods acts as a "proof of concept" for this util function and to serve as a base implementation. I think I did something similar with the other network functions for refactoring how we fetch the release information for each ctmod.

With this change the DXVK and Luxtorpeda ctmods look a lot cleaner in my opinion. The majority of the complexity is now stripped out and replaced behind generic functions. The only remaining complexity in these ctmods really is the boilerplate ctmod stuff and the extraction logic probably being better suited to go behind an __extract_tool method in each ctmod. But it is overall much cleaner now in my opinion!

Concerns

I am not sure if we should fail if we cannot get the file size. Right now we fall over in that case by returning False but let me know if we should just print a warning and continue!

Also, I am unsure if we should have any catches around writing to the file in case it fails. We have this brand new function so we might as well try to make it as safe as possible. We already have some new catches in place.

And I am also concerned that the function may not be as generic as it could be. Please let me know if there is anything I can improve with it!

Remaining work

I think the remaining thing is to add in the ability to take a request's session, i.e. each ctmod's self.rs. But I'll let this be reviewed in its current state first 😄

Apart from that, we can either add other ctmods in this PR or do them in follow-up PRs. I would prefer to do them in follow-up PRs personally but I understand wanting to get them all out of the way in this PR!

---

I hope this is seen as a welcome change. I have an interest in improving things like this around the codebase where possible, and doing it in a maintainable way. Breaking things into generic functions that we can use around the codebase as well as eventually moving things into separate files (such as breaking up util.py a bit more), is something I hope can bring "architectural" improvements to the codebase 😄

ProtonUp-Qt is my reference when I start a personal Python project so any enhancements on the technical side might help others that do the same. And it might also help others get into contributing if we can break apart complexity into smaller re-usable chunks. Plus, this PR actually fixes a small bug, so it has some user-facing impact too!

As usual all feedback is welcome. Thanks!

P.S. The diff for this is more into the additions than deletions, but as this gets extended to more ctmods, the lines removed will outweigh the lines added. The initial additions cost comes from writing the function, which is a bit large.

[sonic2kk]

Based on discussion in #421 the following changes may be desirable:

• Making the progress_callback lambda an optional parameter that defaults to None. I don't think we can pass a blank lambda, i.e. lambda: pass is invalid.
• Catch more network-related errors when making the request instead of just OSError.

I will push the changes to catch more errors because I think this is just general safety, but I'll wait for further review before implementing any other significant changes (unless I spot something egregious that I want to fix).

[sonic2kk]

In 78328f6, I put all the catches in the one except, because we don't actually want to necessarily handle these cases differently we just want to propagate them up. We're printing the {e}rror so I think this approach is fine.

I also moved the call to progress_callback out of the try block, in case progress_callback triggers something we don't want it to trigger our try block necessarily. Now the try/except is limited strictly to catching issues made with the request.

[DavidoTek]

Thanks!

has better logic for grabbing the file size by allowing us to pass a known file size, so we can consistently work out the download progress, even if Content-Length is blank, without needing to resort to len(response.content) which defeats the purpose of streaming the file and stalls the function call

Ah, that's good. I wasn't aware that GitLab doesn't send the content length.

Getting the File Size to Calculate Download Progress

There is one thing that occurred to me, not related to your changes but to the code in general.

We calculate the download progress using the following function. We do it by dividing current_chunk / check_count.

download_progress = int(min(current_chunk / chunk_count * 98.0, 98.0))

If, for some reason, file_size < buffer_size, then int(file_size / buffer_size) will equal zero and current_chunk / chunk_count will cause a ZeroDivisionError.

chunk_count = int(file_size / buffer_size)

It might make sense to change that line:

chunk_count = int(file_size / buffer_size) or 1

If after all this the filesize is still 0, print an error and return False because if we don't have the file size we can't report the download progress, resulting in a broken progress bar (and we are probably using the function wrong in such a scenario).
I am not sure if we should fail if we cannot get the file size. Right now we fall over in that case by returning False but let me know if we should just print a warning and continue!

I wonder how likely such a case is. But yes, I would actually prefer if it only prints the warning an then continues.
People are probably familiar with broken progress bars and will just wait. But if it just cancels every time, that might confuse them.

With this PR, the progress bar works correctly.

Great!

The rationale here is that we modify a lower-risk ctmod for each launcher; DXVK for Lutris and Heroic, Luxtorpeda for Steam.
With this change the DXVK and Luxtorpeda ctmods look a lot cleaner in my opinion.
Apart from that, we can either add other ctmods in this PR or do them in follow-up PRs. I would prefer to do them in follow-up PRs personally but I understand wanting to get them all out of the way in this PR!

I haven't yet seen anything that could break, but still good. We can migrate them slowly once we know it works perfectly.

Also, I am unsure if we should have any catches around writing to the file in case it fails. We have this brand new function so we might as well try to make it as safe as possible. We already have some new catches in place.

The current behavior is that ProtonUp-Qt crashes and shows a warning. That warning dialog might be helpful if there is a permissions error.
I think handling the warning as you suggested is the best solution, but it might make sense to give more fine-grained feedback from the function.
We could do it by returning an int or somehow the error itself. We could forward this error to the __download function of the ctmods and then show a warning dialog from the main window. Any suggestions on how we could do that?

And I am also concerned that the function may not be as generic as it could be. Please let me know if there is anything I can improve with it!

Looks fine to me.

I hope this is seen as a welcome change. I have an interest in improving things like this around the codebase where possible and doing it in a maintainable way.

Yes, that is great. Thanks.

ProtonUp-Qt is my reference when I start a personal Python project so any enhancements on the technical side might help others that do the same. And it might also help others get into contributing if we can break apart complexity into smaller reusable chunks.

That is good to hear 😄

Making the progress_callback lambda an optional parameter that defaults to None. I don't think we can pass a blank lambda, i.e. lambda: pass is invalid.

I think that one might be useful. To keep the changes low we could a hack like this:

if not progress_callback:
    progress_callback = lambda _: None

[sonic2kk]

If, for some reason, file_size < buffer_size, then int(file_size / buffer_size) will equal zero and current_chunk / chunk_count will cause a ZeroDivisionError.

Ah you're right! But sadly I don't think adding an or will fix it. int(0 / 0) or 1 still gives a ZeroDevisionError. We will probably have to do a check ourselves.

I wonder how likely such a case is. But yes, I would actually prefer if it only prints the warning an then continues.
People are probably familiar with broken progress bars and will just wait. But if it just cancels every time, that might confuse them.

I can make that change no problem :-)

The current behavior is that ProtonUp-Qt crashes and shows a warning. That warning dialog might be helpful if there is a permissions error.
I think handling the warning as you suggested is the best solution, but it might make sense to give more fine-grained feedback from the function.
We could do it by returning an int or somehow the error itself. We could forward this error to the __download function of the ctmods and then show a warning dialog from the main window. Any suggestions on how we could do that?

Good point, I think giving an "error code" so to speak is a better approach than True or False. Or we could go the route of raising exceptions. I think either or is fine. If we want to go with an error code we might want to put them in an Enum, which got me thinking at that point we might just be best raising exceptions (and if necessary creating custom ones).

This would allow us to catch and create dialogs for specific errors in the function. We would have to wrap the function call in a try/except block but we'd also probably have to create a bunch of if status == ExitCodes.PERMISSION_ERROR anyway, and I believe Python has exceptions built-in for permission errors.

I think that one might be useful. To keep the changes low we could a hack like this:

I always learn new things when I open PRs here, I had no idea you could create a blank lambda like that, but that works perfectly.

The progress_callback is typed to only take an int argument, but perhaps we could make this hack a little bit safer and make the lambda take any number of arguments, such as lambda *args, **kwargs: None. This would mean if we ever ended up in a case where we needed a more flexible function, we wouldn't need to maintain the blank lambda hack. If we ever called progress_callback(1, 2) for some reason the lambda hack would fail because it only takes one parameter, but with this we would future-proof against any changes to the expected callback signature (fancy word 😄).

I haven't yet seen anything that could break, but still good. We can migrate them slowly once we know it works perfectly.

The PRs, they are a-flowin'. On this note, I haven't forgotten about migrating ctmods to use the other network functions, the remaining ones are just a bit tricky but I do have local proof-of-concepts of moving all ctmods over. Just to clarify that I don't intend to leave that work half-finished nor this work to move other ctmods into using this util function :-)

[sonic2kk]

Made the changes to make the progress_callback lambda optional. Instead of the if not check, I just changed the default value to be the blank lambda 😄

I also pushed the change to simply warn that the progress bar may not display properly if the retrieved filesize is 0.

Finally I also pushed a commit to ensure buffer_size should never be zero to prevent the zero division error when calculating the chunk size. I did this in two ways:

• If buffer_size is zero or less, default to 65536 because this is the default parameter.
  • I thought about checking to make sure the type was an int or float, but I figured that the type hinting was enough. If we're ever not passing this an int we are using the function wrong (because we are the ones that specify buffer_size.
• Wrapped a try/except around the chunk_count calculation, just in case :-) Shouldn't be strictly necessary though.

---

The remaining thing to think about is how to return more fine-grained error messages from the function.

[DavidoTek]

Great, thanks.

The remaining thing to think about is how to return more fine-grained error messages from the function.

I think we have three options:

1. Throw errors: Don't handle the errors in the first place
2. Throw errors: Catch the errors in download_file and throw a new, predefined set of errors there
3. Return an Int / Enum (0=no error) - C-like
4. Return an str|None (None=no error) - Go-like

The advantage of 3 and 4 is that the program will continue even if errors are not being handled. But I do not think this is a wanted behavior because we cannot work with a file that wasn't downloaded successfully.
I think 1 or 2 are the way to go. 2 has the advantage that we know exactly which errors may be thrown. Additionally, we can narrow the number of possible arrows. I feel 1 doesn't make much sense as the purpose of the function is to make the error handling easier. What do you think?

If we go with 2, the question is which errors we should use. We can use Python standard errors for that with a custom message. We need something for Internet-related errors (connection/endpoint/timeout errors) and local errors (permission, path does not exist, ...). Not sure if we should also handle generic errors such as "expected int, received str" because this kind of error should be found by the CI/unit testing and not happen sporadic when the user uses ProtonUp-Qt.

[sonic2kk]

Hmm, I do think 2 is the way to go ultimately but, do we really not want ProtonUp-Qt to continue if a download fails? If we can't download a compatibility tool, should it really crash?

To go back to the thing that sparked this:

The current behavior is that ProtonUp-Qt crashes and shows a warning. That warning dialog might be helpful if there is a permissions error.
I think handling the warning as you suggested is the best solution, but it might make sense to give more fine-grained feedback from the function.
We could do it by returning an int or somehow the error itself. We could forward this error to the __download function of the ctmods and then show a warning dialog from the main window. Any suggestions on how we could do that?

The problem we want to solve here is to catch and bubble errors up to whatever calls download_file. We are catching some errors at the moment, such as connection failures and timeouts. But when writing the streamed data to a file we may hit issues, and we want to be able to catch that and other problems in a more fine-grained way.

With Option 2 proposed, we would catch the many errors that could happen and return a new error for fine-grained control from the caller. For example with all of the network problems we could hit we could simply return some kind of custom NetworkError or something, and handle this.

I am not sure what the best pattern is in Python for writing error handling paths (or to be honest, in most languages 😅).

Not sure if we should also handle generic errors such as "expected int, received str" because this kind of error should be found by the CI/unit testing and not happen sporadic when the user uses ProtonUp-Qt.

I agree, I think these kinds of errors are out of scope here. We want to focus on catching issues with something going wrong "externally" i.e. from downloads and when writing.

---

So I think we should focus on catching the errors in download_file from external actions only, not so much from our own potential syntax errors. I'm just not sure what pattern we should use to achieve Option 2. Do we want to create and raise custom errors, and if so, how do we want to use those? I don't remember what the vision was here. Maybe we do want to fully stop execution and raise a custom error from the except blocK? Something like this:

try:
# Try to get the data for the file we want
try:
    response: requests.Response = requests.get(url, stream=stream)
except (OSError, requests.ConnectionError, requests.Timeout) as e:
    print(f'Error: Failed to make request to URL {url}, cannot complete download! Reason: {e}')
    raise pupgui2.NetworkError  # Custom error that we create

[DavidoTek]

We definitively should catch the errors at a higher level then, show a message box and continue operation normally.

I am not sure what the best pattern is in Python for writing error handling paths (or to be honest, in most languages 😅).

We could just do something like this, but I don't think that is very Pythonic. Throwing a less fine grained error and handling it at a higher level seems like a better solution.

def just_return_error():
    try:
        raise OSError("An error occured.")
    except Exception as e:
        return e

# Option 2
def throw_new_error():
    """ Throws: RuntimeError """
    try:
        raise OSError("An error occured.")
    except Exception as e:
        raise RuntimeError("Some error related to the OS occured.")

I kinda like how Rust and Go handle this.
In Rust, you have the Result type which can either be Ok(T) or Err(E) where T is the desired type and E is some error type.
In Go, functions usually return a tuple like (str, Error) and you follow each function with if err != nil {...} (which result in a lot of boilerplate code...)

I'm just not sure what pattern we should use to achieve Option 2. Do we want to create and raise custom errors, and if so, how do we want to use those? I don't remember what the vision was here. Maybe we do want to fully stop execution and raise a custom error from the except blocK? Something like this:

The alternative, which I feel makes a bit more sense when we want to use Python runtime errors is to not actually handle errors inside the download_file function and just handle them one layer above in the ctmod where we can display a message box.
I don't think we can display a message box from inside the download_file function that easily and creating a Signal/callback just for that purpose feels wrong IMO.

[sonic2kk]

I kinda like how Rust and Go handle this.

I'm not too familiar with Go, but Rust's approach to error handling is very interesting. I have been playing around with Rust in my own time and I do quite like it 😄 (inb4 there are 500 discussions opened about rewriting ProtonUp-Qt in Rust)

The alternative, which I feel makes a bit more sense when we want to use Python runtime errors is to not actually handle errors inside the download_file function and just handle them one layer above in the ctmod where we can display a message box. I don't think we can display a message box from inside the download_file function that easily and creating a Signal/callback just for that purpose feels wrong IMO.

Yeah, it seems a bit silly to make a signal specifically for showing a messagebox.

My experience handling exceptions is very sparse, apologies if this is a bit silly but, should we remove all of the try/except blocks which return False from download_file, or keep them but raise the exceptions so that we can additionally handle them in the ctmods (or whatever else might call download_file)?

I guess it isn't wholly necessary to keep the try/excepts that we can't handle in download_file, apart from being able to log inside of the function itself, if the caller doesn't wrap it properly or something.

Are there any good practices for noting in docstrings / elsewhere what exceptions this function might raise? In case a caller wants to more specifically handle each exception (we can always look at the code, but still :-))

[sonic2kk]

I pushed a change to raise the exceptions in download_file and a proof-of-concept of handling them in the ctmods. I commented out some code to create a dialog because it was causing segfaults for some reason (the logic in ctloader wasn't). I remember hitting something similar a long time back with the SteamTinkerLaunch ctmod but I don't remember what I had to fix, maybe it was a threading issue? It's late and I can't figure it out right now 😅

Also rebased the branch on top of main specifically to pull in the newer dependencies and the tests.

Sorry if this is spawning another discussion but should I include a test for download_file in this PR? It might be a bit more "complex" compared to the other tests we currently have as we'd probably need to do some kind of mocking I think to simulate hitting a request. I'd have to do a bit of research on how to properly test functions that perform network actions but that could be a good thing as it would give us a reference implementation if we need to do this in future (i.e. for the AWACY list). Plus I've been harping on about making this function safe, and I proposed adding tests, so it seems fair, but it can also go in a follow-up PR if you prefer.

[DavidoTek]

Great.

Are there any good practices for noting in docstrings / elsewhere what exceptions this function might raise?

There isn't really a signle standard for this and we aren't following any of the few conventions either.
I guess the one closest is this one which just adds a Raises section similar to the arguments/parameters section: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

Sorry if this is spawning another discussion but should I include a test for download_file in this PR?

Feel free to add it in this request if you want to.

I'd have to do a bit of research on how to properly test functions that perform network actions but that could be a good thing as it would give us a reference implementation if we need to do this in future (i.e. for the AWACY list).

You may take a look at https://requests-mock.readthedocs.io/en/latest/
I don't know how much you can control the mocks, like not sending a content length, for example.

[sonic2kk]

Okay, wired up the error dialogs for Luxtorpeda and DXVK. They now look like this, I forced the download to fail by setting the URL to something invalid when calling download_file:

Might tweak the dialog title very slightly to note a download error or something but I think it's good to have this kind of notification!

---

I will take a stab at adding a test for the download_file function. Thanks for the heads up on requests-mock, we would be adding another dependency if we introduced this but maybe as discussed in another PR, a requirements_dev.txt is in order if we end up with a lot of these kinds of cases... I'll see how far I get with regular ol' PyTest.

EDIT: Another potential library of interest is pytest-mock: https://pytest-mock.readthedocs.io/en/latest/usage.html

[sonic2kk]

Pushed the changes to make the error dialog strings translatable, and it looks like the dialog is still working fine :-)

I originally made these separate variables, but figured it was fine to just change them to be inline in the function parameters but with newlines.

---

As for the test for this function, I'm still looking into it, trying to learn some of the best practices for PyTest, how to use things like fixtures, the responses library and so on. If you'd prefer to merge this sooner than later we can merge as-is and I can look into a test in another PR.

[DavidoTek]

Thanks.

I've added a few commits:

• Remove the "separate variables" also for dxvk
• Replaced the single quotes with double quotes (f' ... '{url}' ...' won't work)
• Cleaned the imports (I noticed they have become quite a lot, we probably can do something similar for all files)

If you'd prefer to merge this sooner than later we can merge as-is and I can look into a test in another PR.

Okay, I will merge this now as it seems to work fine. We can concentrate on the testing in a later PR.

---

Remarks:

• Something we should consider (and add to the future CONTRIBUTING.md): Should all PRs that add new features always have 100% test coverage/contain the tests? (maybe it is good enough to decide for each case)
• If we add an interface class for all CtInstallers, we can add the translated strings there as they are the same for all compatibility tools.

[sonic2kk]

Should all PRs that add new features always have 100% test coverage/contain the tests? (maybe it is good enough to decide for each case)

This is something that I've seen discussed around in general around testing, what is the "ideal" test coverage? The only consistent thing I have seen is that 100% coverage doesn't have to be the goal, because there's no need to end up writing tests just to tick a box. Tests are there to give you confidence in the code you write and help spot/avoid defects.

I think this is something best decided on a case-by-case basis. I don't know if there's a good way to outline fully what new features may and may not require a test. On one hand something trivial may not require a test, but on the other hand if it's so trivial it may be straightforward to write a test for.

I don't have a good answer :-) But maybe overtime we'll figure out a good pattern!