Code quality improvements: clarify typing, improve documentation #262
Conversation
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Typing was incorrect, name was unclear Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
…nts, and changed cfbs command functions with `git_magic` decorators to always return `Result` before decoration Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
This fixes Python type checking tooling being confused about return types of functions decorated by the `git_magic` decorator - apparently, an explicit return type hint is needed for correct functioning even when the return type is correctly inferred Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
The command functions never return `Result`, and pyright now understands that. Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
The former name was highly generic and did not explain the purpose of the class. Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
This is useful in case more helper types are added. Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
The explicit type hints have been removed because they are redundant and could be a conflicting second source of truth Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
jakub-nt
force-pushed
the
code-improvements-types
branch
from
4980548 to
2d7b745
Compare
larsewi
left a comment
larsewi
left a comment
There was a problem hiding this comment.
Thanks for working on this 🚀
| def search_command(terms: List[str]): | ||
| The return type (after any decoration) of the cfbs command functions is `CFBSCommandExitCode`. | ||
| This type is indistinguishably defined to be `int`. |
There was a problem hiding this comment.
| This type is indistinguishably defined to be `int`. | |
| This type is defined to be `int`. |
There was a problem hiding this comment.
There are two ways to define a type like that, one where you can pass the underlying type as the type and there's no type error, and one where there is a type error and you have to explicitly construct an instance of the type. The point behind this word was to highlight which one of the two this is (the former).
| def search_command(terms: List[str]) -> int: | ||
| def search_command(terms: List[str]): | ||
| The return type (after any decoration) of the cfbs command functions is `CFBSCommandExitCode`. |
There was a problem hiding this comment.
Defined a CFBSCommandExitCode type to improve maintainability
How does this improve maintainability?
There was a problem hiding this comment.
It's similar to a constant variable. For example, suppose one wanted to redefine the type from int to something else, then without this definition, one would have to find the several places that would need changing, and replace the type in every single one. With this definition, one can just replace it once in the variable definition.
There was a problem hiding this comment.
On the other hand, it could be argued that the extra obfuscation / indirection makes it harder to read, not easier. Usually, making sure it's easy to read / understand is more important than making it easy to change, but YMMV.
3 participants
No description provided.