Remove unused code, rework type hints #730
Conversation
Conventional Commits Report
🚀 Conventional commits found. |
NiklasHargarter
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
+ Coverage 79.51% 79.56% +0.04%
==========================================
Files 86 86
Lines 2910 2917 +7
Branches 611 611
==========================================
+ Hits 2314 2321 +7
- Misses 450 451 +1
+ Partials 146 145 -1 ☔ View full report in Codecov by Sentry. |
NiklasHargarter
changed the title
| @@ -18,13 +18,17 @@ | |||
| """ Argument parser for troubadix """ | |||
|
|
|||
| import sys | |||
| from argparse import ArgumentParser, Namespace | |||
| import warnings | |||
There was a problem hiding this comment.
We already have the pontos.Terminal with warn or warning
| def check_cpu_count(number_str: str) -> int: | ||
| """ | ||
| Ensures the given value is a valid CPU core count. | ||
|
|
||
| Args: | ||
| number_str (str): The number of CPU cores as a string. | ||
|
|
||
| Returns: | ||
| int: A valid CPU core count. | ||
| """ | ||
| try: | ||
| number = int(number_str) | ||
| except ValueError as e: | ||
| raise ArgumentTypeError( | ||
| f"Invalid number of jobs: '{number_str}' is not an integer." | ||
| ) from e | ||
| if number > MAX_JOB_COUNT: | ||
| warnings.warn( | ||
| f"Requested number of jobs ({number}) " | ||
| "exceeds available CPU cores ({MAX_JOB_COUNT}). " | ||
| f"Using maximum available: {MAX_JOB_COUNT}.", | ||
| ) | ||
| return MAX_JOB_COUNT | ||
| if number < 1: | ||
| return max_count // 2 | ||
| warnings.warn( | ||
| f"Requested number of jobs ({number}) is less than 1. " | ||
| f"Using default: {DEFAULT_JOB_COUNT}.", | ||
| ) | ||
| return DEFAULT_JOB_COUNT | ||
| return number |
There was a problem hiding this comment.
Tbh I prefer the previous version more.
Moving the MAX_JOB_COUNT and DEFAULT_JOB_COUNT variables out is OK, but the rest seems a bit over engineered to me
| @@ -216,11 +238,11 @@ def parse_args( | |||
| "-j", | |||
| "--n-jobs", | |||
| dest="n_jobs", | |||
| default=max(1, cpu_count() // 2), | |||
| default=str(DEFAULT_JOB_COUNT), | |||
There was a problem hiding this comment.
| default=str(DEFAULT_JOB_COUNT), | |
| default=DEFAULT_JOB_COUNT, |
int(<int as input>) just returns that int, so this should be cool / less head scratchy why the default it str
| type=check_cpu_count, | ||
| help=( | ||
| "Define number of jobs, that should run simultaneously. " | ||
| "Default: %(default)s" | ||
| f"Default: {DEFAULT_JOB_COUNT} (half of the available CPU cores)" |
There was a problem hiding this comment.
| f"Default: {DEFAULT_JOB_COUNT} (half of the available CPU cores)" | |
| f"Default: {DEFAULT_JOB_COUNT} (half of the available CPU cores)" |
Please revert this, as the previous implementation will always point to the default value of this argument
| return number | ||
|
|
||
|
|
||
| def parse_args( | ||
| terminal: Terminal, | ||
| args: Iterable[str] = None, | ||
| args: Sequence[str], |
There was a problem hiding this comment.
Why is Sequence better here than Iterable ?
|
|
||
| @abstractmethod | ||
| def run(self) -> Iterator[LinterResult]: | ||
| pass | ||
|
|
||
| def fix(self) -> Iterator[LinterResult]: | ||
| return [] | ||
| return iter([]) |
There was a problem hiding this comment.
Does this make anything better / fixes a bug somewhere ?
| self.root = root | ||
| self.nasl_files = nasl_files | ||
|
|
||
|
|
||
| class Plugin(ABC): | ||
| """A linter plugin""" | ||
|
|
||
| name: str = None | ||
| description: str = None |
What
Changes type hints used in core modules. Functionality is unchanged.
In limited cases, changes have been made to things other than type hints so that a type hint is correct.
Iterable is replaced by list. This is because only lists are ever used and passed to methods that are typed with iterable. Iterable allows more abstraction that is never used and unnecessarily hides what is actually being passed. So I think using list makes the code cleaner.
The use of a non-optional type with a default value of None, e.g. name: str = None, is either changed to Optional[type] or the default of None is removed. This depends on whether None is actually a valid value for that variable/argument, and whether a default value makes sense.
Imports from typing that are deprecated in 3.9, such as List, are replaced with what typing recommends to use.
Removed plugin.description as I could not find a use for it anywhere and no plugin overwrites it.
Unclear if can be removed
Troubadix.main method default args
exists so that Troubadix main can be called internally or probably imported as a module and not only run exclusively from the command line.
Why
So that the typing hint system is correct and makes future development easier.
References
Checklist