Reduced the type: ignore comments and allocated the structured dataclass #393

vinothk-master · 2025-02-22T17:29:03Z

Added these set of lines which is used to reduce the type :ignore comments in compare.py file 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 : SpecsServiceDict

Thanks
Vinoth Kannan

CLA-Mercedes-Benz · 2025-02-22T17:29:09Z

All committers have signed the CLA.

kayoub5 · 2025-02-22T18:23:26Z

odxtools/cli/compare.py


 # name of the tool
 _odxtools_tool_name_ = "compare"
-
+"""


commented out code

kayoub5 · 2025-02-22T18:23:58Z

odxtools/cli/compare.py

+    deleted_variants : List[str]
+    service_changes : SpecsServiceDict
+#class extract_service_tabulation_data(TypedDict):
+#    table: RichTable


commented out code

kayoub5 · 2025-02-22T18:24:27Z

odxtools/cli/compare.py

-                "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)


commented out code

andlaus

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...

andlaus · 2025-02-24T09:16:38Z

odxtools/cli/compare.py


-SpecsServiceDict = Dict[str, Union[VariantName, VariantType, NewServices, DeletedServices,
-                                   RenamedServices, ServicesWithParamChanges]]
+class SpecsServiceDict(TypedDict):


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.)

Modified the code as per the comment,

@dataclass class ServiceSpecs: 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]]]] @dataclass class SpecsChangesVariants: new_variants: List[str] deleted_variants: List[str] service_changes: ServiceSpecs

andlaus · 2025-02-24T09:22:53Z

odxtools/cli/compare.py

-                "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"]


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...

Modified as service_dict variable to service_spec but the mypy couldn't handle it... It throws the error.

andlaus · 2025-02-24T09:23:49Z

odxtools/cli/compare.py

+            or service_dict["changed_name_of_service"]
+            or service_dict["changed_parameters_of_service"]
+        ):
+            #   assert isinstance(service_dict["diag_layer"], str)


please remove such obsolete code instead of commenting it out

Removed the obsolete code.

andlaus · 2025-02-24T09:26:19Z

odxtools/cli/compare.py

-                    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


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, I debugged the magical number 2 by running the test on my local machine and found that it is a list,

service_spec["changed_parameters_of_service"][2]: [[" Properties of positive response parameter 'status' that have changed:\n", {'Property': ['Bit Length', 'Linked DOP object', ' DOP name'], 'Old Value': [16, '', 'uint16'], 'New Value': [8, '', 'uint8']}], [" Properties of positive response parameter 'num_flips_done' that have changed:\n", {'Property': ['Byte position'], 'Old Value': [2], 'New Value': [None]}]]

In the above case the magical number 2 is mandatory to print the detailed table. Please let me know if there is anything I can change?

Thanks

yes, it's things like this that make the compare tool code so difficult to percolate: in this case the second element of the list passed to print_dl_changes() represents the detailed information about what changed with the parameters (cf. https://github.com/mercedes-benz/odxtools/blob/main/odxtools/cli/compare.py#L493-L496 ). IMO it would be great if this was immediately obvious by looking at the code, e.g. the above should become service_spec.changed_parameters[param_idx].change_details with the change_details attribute being a proper dataclass as well...

vinothk-master · 2025-02-24T21:15:08Z

Member

Overall executed the reformat-source.sh as a result it would satisfy the formatting conditions.

$ sh reformat-source.sh
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\basicstructure.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\nameditemlist.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\odxtypes.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\request.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\response.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\cli\compare.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\odxtools\diaglayers\hierarchyelement.py
Fixing E:\OpenSource Contributions\Mercedes\odxtools\tests\test_compu_methods.py

…rmat-source.sh etc.

andlaus · 2025-02-25T10:59:41Z

Overall executed the reformat-source.sh as a result it would satisfy the formatting conditions.

yes. ideally you run this script before you commit anything, so you don't need to worry about the formatting...

andlaus · 2025-02-25T11:01:38Z

(before you push to github, it is IMO advisable to run mypy and ruff as well because these are build into the CI and your PR will fail if these two do not pass locally...)

vinothk-master · 2025-02-25T11:05:36Z

(before you push to github, it is IMO advisable to run mypy and ruff as well because these are build into the CI and your PR will fail if these two do not pass locally...)

Ok let me work on it and thanks for letting me know

vinothk-master added 2 commits February 22, 2025 17:19

Reduced the type: ignore comments and allocated the structured dataclass

86b1ae5

Reduced the type: ignore comments and allocated the structured dataclass

38dec1f

kayoub5 reviewed Feb 22, 2025

View reviewed changes

odxtools/cli/compare.py Outdated

# name of the tool

_odxtools_tool_name_ = "compare"

"""

Copy link

Collaborator

kayoub5 Feb 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented out code

kayoub5 reviewed Feb 22, 2025

View reviewed changes

fixed the lint issues

824a499

andlaus reviewed Feb 24, 2025

View reviewed changes

fixed the issues : change the name to service_spec, executed the refo…

716acc5

…rmat-source.sh etc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduced the type: ignore comments and allocated the structured dataclass #393

Reduced the type: ignore comments and allocated the structured dataclass #393

vinothk-master commented Feb 22, 2025

CLA-Mercedes-Benz commented Feb 22, 2025 •

edited

Loading

kayoub5 Feb 22, 2025

kayoub5 Feb 22, 2025

kayoub5 Feb 22, 2025

andlaus left a comment

andlaus Feb 24, 2025 •

edited

Loading

vinothk-master Feb 24, 2025

andlaus Feb 24, 2025

vinothk-master Feb 24, 2025

andlaus Feb 24, 2025

vinothk-master Feb 24, 2025

andlaus Feb 24, 2025

vinothk-master Feb 24, 2025

andlaus Feb 25, 2025 •

edited

Loading

vinothk-master commented Feb 24, 2025

andlaus commented Feb 25, 2025

andlaus commented Feb 25, 2025

vinothk-master commented Feb 25, 2025

Reduced the type: ignore comments and allocated the structured dataclass #393

Are you sure you want to change the base?

Reduced the type: ignore comments and allocated the structured dataclass #393

Conversation

vinothk-master commented Feb 22, 2025

CLA-Mercedes-Benz commented Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andlaus left a comment

Choose a reason for hiding this comment

andlaus Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andlaus Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

vinothk-master commented Feb 24, 2025

andlaus commented Feb 25, 2025

andlaus commented Feb 25, 2025

vinothk-master commented Feb 25, 2025

CLA-Mercedes-Benz commented Feb 22, 2025 •

edited

Loading

andlaus Feb 24, 2025 •

edited

Loading

andlaus Feb 25, 2025 •

edited

Loading