Admin: Add typehints to utils/strings and utils/threads #13658
Conversation
Test Results - Preflight, Unit23 088 tests +4 21 229 ✅ +4 6m 11s ⏱️ -15s Results for commit b66207d. ± Comparison against base commit 08f7836. This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| from localstack.utils.objects import recurse_object | ||
|
|
||
| if isinstance(value, (dict, list)): | ||
| if isinstance(value, str): |
There was a problem hiding this comment.
The logic here is unchanged - just flipped to test the smallest type first. This was necessary because ComplexType includes the type object, and strings are a type of object. Ergo: isinstance("x", ComplexType) would resolve to True, and that's not what we want.
| if argspec.varkw or "_thread" in (argspec.args or []) + (argspec.kwonlyargs or []): | ||
| kwargs["_thread"] = self |
There was a problem hiding this comment.
This.. thing was only used in one place, but that usage has now been refactored (see related PR), so this wart is no longer necessary.
5a7f998 to
36bbc81
Compare
a563d1f to
fcde7b7
Compare
| if argspec.varkw or "_thread" in (argspec.args or []) + (argspec.kwonlyargs or []): | ||
| kwargs["_thread"] = self | ||
| kwargs = {} # type: ignore[var-annotated] | ||
| result = self.func(self.params, **kwargs) |
There was a problem hiding this comment.
Because we always pass self.params, usage currently looks like this:
def run_install(*args):
assert args == (None,)
start_worker_thread(run_install)In an ideal world, we would not need to define any arguments:
def run_install():
pass
start_worker_thread(run_install)and only pass in any parameters if configured:
def run_install(arg1, arg2):
pass
start_worker_thread(run_install, params=("arg1", "arg2"))However, this would include a refactoring of all the usages of this pattern across the codebase, so I'm just leaving this as-is for now.
|
|
||
|
|
||
| def convert_to_printable_chars(value: list | dict | str) -> str: | ||
| def convert_to_printable_chars(value: "str | ComplexType") -> "ComplexType": |
There was a problem hiding this comment.
I used ComplexType here just because it's an existing type that accurately describes the possibilities.
For reference, this is what ComplexType looks like:
ComplexType = list | dict | object
This does need more attention later on, but that's not a rabbit hole I'm willing to dive into right now 🙂
fcde7b7 to
b66207d
Compare
alexrashed
left a comment
There was a problem hiding this comment.
Nice cleanup of what's some really historical code! 💯
The changes are looking great, some nice cleanups in there and the changes should be functional equivalent besides the changes you outlined (and which should be safe as well).
I think it might be a good idea to quickly verify that these changes really don't cause any issues in our downstream project, but then this should be good to go! 🚀 
|
Thanks again for the review @alexrashed - can confirm that this does not cause any issues downstream 👍 |
Motivation
This PR adds typehints to some utilities (
utils/strings.pyandutils/threads.py) and explicitly verifies the correctness usingmypy.The original intention was to only add types to the most common methods, like
to_strandstart_thread- but adding types to the entire file wasn't that much more effort, that's why I went all out.Some logic has been moved around - I'll add some inline comments to explain what happened, and why this is (IMHO) acceptable.
The (single) test that we had for
FuncThreadshas also been reworked and moved to a dedicated testfile, with some additional tests for good measure.Not sure who actually owns these utilities! @alexrashed I've tagged you as the resident Python guru, but feel free to re-assign if you think someone else is more suitable 🙂