Reduced the type: ignore comments and allocated the structured dataclass #393
Conversation
|
|
odxtools/cli/compare.py
Outdated
|
|
||
| # name of the tool | ||
| _odxtools_tool_name_ = "compare" | ||
|
|
||
| """ |
odxtools/cli/compare.py
Outdated
| deleted_variants : List[str] | ||
| service_changes : SpecsServiceDict | ||
| #class extract_service_tabulation_data(TypedDict): | ||
| # table: RichTable |
odxtools/cli/compare.py
Outdated
| "changed_name_of_service"][0] or service_dict["changed_parameters_of_service"][0]: | ||
| assert isinstance(service_dict["diag_layer"], str) | ||
| "changed_name_of_service"] or service_dict["changed_parameters_of_service"]: | ||
| # assert isinstance(service_dict["diag_layer"], str) |
There was a problem hiding this comment.
In principle, I like it. IMO -- besides the usual bits and pieces that need to be fixed -- for whatever reason it seems that a lot of unrelated formatting changes slipped into this (which makes reviewing it more difficult than it needs to be and also prevents the CI from passing). To fix the formatting, install yapf and isort via pip and run the reformat-source.sh script in the toplevel directory of your working copy...
|
|
||
| SpecsServiceDict = Dict[str, Union[VariantName, VariantType, NewServices, DeletedServices, | ||
| RenamedServices, ServicesWithParamChanges]] | ||
| class SpecsServiceDict(TypedDict): |
There was a problem hiding this comment.
IMO that should better be an @dataclass (and be renamed to ServiceSpecs) instead of being a TypedDict. Maybe I'm missing something, though. (the same applies to the other TypedDict derivatives below.)
| "changed_name_of_service"][0] or service_dict["changed_parameters_of_service"][0]: | ||
| assert isinstance(service_dict["diag_layer"], str) | ||
| if ( | ||
| service_dict["new_services"] |
There was a problem hiding this comment.
if SpecsServiceDict becomes a dataclass, this should change this to service_spec.new_services (assuming you rename the service_dict variable to service_spec). I'm not sure if mypy can handle typed dictionaries properly, but IMO directly accessing the attributes is considerably less clunky...
| or service_dict["changed_name_of_service"] | ||
| or service_dict["changed_parameters_of_service"] | ||
| ): | ||
| # assert isinstance(service_dict["diag_layer"], str) |
There was a problem hiding this comment.
please remove such obsolete code instead of commenting it out
| list, # type: ignore[type-arg] | ||
| service_dict["changed_parameters_of_service"])[2][service_idx] | ||
| info_list = cast(list, service_dict["changed_parameters_of_service"])[ | ||
| 2 |
There was a problem hiding this comment.
that 2 seems too much of a magical number to me. Ideally, this would be something like
service_spec.changed_parameters.added_params (or whatever the third entry of that list represents)
Hi @andlaus
Added these set of lines which is used to reduce the
type :ignorecomments incompare.pyfile which increases the readability and to maintain a structured dict. Please check these line commits and let me know I can help on this regard.class SpecsServiceDict(TypedDict): diag_layer: str diag_layer_type: str new_services: List[str] # List of new service names deleted_services: List[DiagService] # List of deleted services renamed_service: List[Tuple[str, DiagService]] # List of (old_name, DiagService) changed_name_of_service: List[Tuple[str, DiagService]] # Similar to renamed_service changed_parameters_of_service: List[Tuple[str, DiagService, List[Union[str, int, float]]]] class SpecsChangesVariants(TypedDict): new_variants : List[str] deleted_variants : List[str] service_changes : SpecsServiceDictThanks
Vinoth Kannan