Skip to content
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
86b1ae5
Reduced the type: ignore comments and allocated the structured dataclass
vinothk-master Feb 22, 2025
38dec1f
Reduced the type: ignore comments and allocated the structured dataclass
vinothk-master Feb 22, 2025
824a499
fixed the lint issues
vinothk-master Feb 22, 2025
716acc5
fixed the issues : change the name to service_spec, executed the refo…
vinothk-master Feb 24, 2025
89f91f2
Merge branch 'main' of https://github.com/vinothk-master/odxtools int…
vinothk-master Mar 22, 2025
22241df
refractored the code by adding dataclass and formated using reformat.sh
vinothk-master Mar 23, 2025
4da9b80
Merge branch 'mercedes-benz:main' into refractor_compare_tool
vinothk-master Mar 23, 2025
c6e45d7
refractored the code by adding dataclass and formated using reformat.…
vinothk-master Mar 23, 2025
cf1288b
refractored the code by adding dataclass and formated using reformat.…
vinothk-master Mar 23, 2025
87de2a2
Merge branch 'refractor_compare_tool'
vinothk-master Mar 23, 2025
c32a2cf
refractored the code by adding dataclass and formated using reformat.…
vinothk-master Mar 23, 2025
4d35bd9
refractored the code by adding dataclass and formated using reformat.sh
vinothk-master Mar 23, 2025
7f75ca4
Merge branch 'main' of https://github.com/vinothk-master/odxtools int…
vinothk-master Mar 23, 2025
e7bfc30
Refractoring the code base with dataclass
vinothk-master Mar 27, 2025
161bf41
adding to remote branch
vinothk-master Mar 28, 2025
ef7660e
Remove odxtools/cli/.gitignore from the repository
vinothk-master Mar 28, 2025
9f1c890
Removing the obselete codes
vinothk-master Mar 28, 2025
0c9f2fe
Merge branch 'mercedes-benz:main' into main
vinothk-master Mar 28, 2025
8cd4344
updating the branch
vinothk-master Mar 28, 2025
3e4d1b2
removing auto-generated non-source files
vinothk-master Mar 30, 2025
28aa654
Merge branch 'mercedes-benz:main' into main
vinothk-master Mar 30, 2025
1485d00
Merge branch 'main' of https://github.com/vinothk-master/odxtools int…
vinothk-master Mar 30, 2025
22cef5b
removing the Any type and removing the obselete lines
vinothk-master Mar 30, 2025
4940cfa
adding the structured code base
vinothk-master Apr 3, 2025
2f742b5
refactor the compare tool to reduce the number of ype: ignore comments
vinothk-master Apr 16, 2025
7198894
Adding the changes
vinothk-master Jul 23, 2025
c68064b
improving both the compare and printutils files
vinothk-master Aug 12, 2025
fbf5f0e
Merge pull request #4 from mercedes-benz/main
vinothk-master Aug 12, 2025
00e1938
Adding the changes and refractored the code base on compare.py and _p…
vinothk-master Aug 13, 2025
a52a80d
Merge branch 'main' of https://github.com/vinothk-master/odxtools int…
vinothk-master Aug 13, 2025
08cd9b4
adding the changes to subbranch
vinothk-master Aug 13, 2025
ad689f2
Merge branch 'main' of https://github.com/vinothk-master/odxtools
vinothk-master Aug 13, 2025
6df77e8
implementing the above changes
vinothk-master Aug 13, 2025
4c91a17
Merge branch 'main' into improve_compare_tool
vinothk-master Aug 13, 2025
d18b9e5
Merge pull request #5 from vinothk-master/improve_compare_tool
vinothk-master Aug 13, 2025
96fd6f1
Merge branch 'mercedes-benz:main' into improve_compare_tool
vinothk-master Aug 27, 2025
9bf55fd
Merge branch 'mercedes-benz:main' into main
vinothk-master Aug 27, 2025
65a3a0f
Merge branch 'improve_compare_tool' of https://github.com/vinothk-mas…
vinothk-master Aug 27, 2025
252a385
save work
vinothk-master Sep 2, 2025
4b9fe24
save work before rebase
vinothk-master Sep 2, 2025
71ca9dd
merged changes
vinothk-master Sep 2, 2025
cd58b64
Merge pull request #7 from vinothk-master/new_refractored_codebase
vinothk-master Sep 2, 2025
78f9574
Merge branch 'main' into improve_compare_tool
vinothk-master Sep 2, 2025
34ba751
Linting the codebase
vinothk-master Sep 2, 2025
e8794bf
Merge branch 'improve_compare_tool' of https://github.com/vinothk-mas…
vinothk-master Sep 2, 2025
b586708
Merge branch 'improve_compare_tool'
vinothk-master Sep 2, 2025
683d62b
Removed copy.py
vinothk-master Sep 3, 2025
21b9550
removing pdx_files_folder
vinothk-master Sep 3, 2025
2127790
Apply yapf formatting
vinothk-master Sep 3, 2025
a197504
Linting the files
vinothk-master Sep 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions odxtools/cli/_print_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,3 +299,33 @@ def print_dl_metrics(variants: list[DiagLayer]) -> None:
str(len(ddds.data_object_props)),
str(len(getattr(variant, "comparams_refs", []))))
rich_print(table)


def print_change_metrics(metrics_results: list[dict[str, object]]) -> None:
"""Print new/deleted/renamed/changed counts between files."""

table = RichTable(
title="Change Metrics",
show_header=True,
header_style="bold cyan",
border_style="blue",
show_lines=True)
table.add_column("File Comparison", style="green")
table.add_column("Diag Layer", style="magenta")
table.add_column("Variant Type", style="magenta")
table.add_column("Variants Added", justify="right", style="yellow")
table.add_column("Variants Changed", justify="right", style="yellow")
table.add_column("Variants Deleted", justify="right", style="yellow")
table.add_column("Services Added", justify="right", style="yellow")
table.add_column("Services Changed", justify="right", style="yellow")
table.add_column("Services Deleted", justify="right", style="yellow")
table.add_column("Changed Parameters", justify="right", style="yellow")
#
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Variants and Diag Layer refer to the same object, therefore I wouldn't print the comparison details for each Diag Layer. See suggestion for table design here.

Copy link
Copy Markdown
Contributor Author

@vinothk-master vinothk-master Sep 3, 2025

Choose a reason for hiding this comment

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

Hi @kakoeh,

Thanks for the input, and I have made the changes as per the requirement.

Thanks
Vinoth

for m in metrics_results:
file_pair = f"{m['file_a']} --> {m['file_b']}"
table.add_row(file_pair, str(m["diag_layer"]), str(m["diag_layer_type"]),
str(m["num_variants_added"]), str(m["num_variants_changed"]),
str(m["num_variants_deleted"]), str(m["num_new_services"]),
str(m["num_deleted_services"]), str(m["num_renamed_services"]),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the column headers, the order is Services Added, Services Changed, Services Deleted. Moreover, both renamed services and services with parameter changes should be characterized as Services Changed from my point of view.

Suggested change
str(m["num_deleted_services"]), str(m["num_renamed_services"]),
str(m["num_renamed_services"]+m["num_changed_parameters"]),
str(m["num_deleted_services"]))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @kakoeh
Thanks for the input and I have made the changes as per the requirement.

Thanks
Vinoth

str(m["num_changed_parameters"]))
rich_print(table)
136 changes: 118 additions & 18 deletions odxtools/cli/compare.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# SPDX-License-Identifier: MIT

import argparse
import json
import os
from dataclasses import dataclass, field
from typing import Any
Expand All @@ -22,7 +23,7 @@
from ..parameters.valueparameter import ValueParameter
from . import _parser_utils
from ._parser_utils import SubparsersList
from ._print_utils import (extract_service_tabulation_data, print_dl_metrics,
from ._print_utils import (extract_service_tabulation_data, print_change_metrics, print_dl_metrics,
print_service_parameters)

# name of the tool
Expand Down Expand Up @@ -303,7 +304,7 @@ def compare_services(self, service1: DiagService, service2: DiagService) -> list
else:
changed_params += "request parameter list, "
# infotext
information.append(f"List of request parameters for service '{service2.short_name}' "
information.append(f"list of request parameters for service '{service2.short_name}' "
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
f"is not identical.\n")

# table
Expand All @@ -312,7 +313,7 @@ def compare_services(self, service1: DiagService, service2: DiagService) -> list
param_list2 = [] if service2.request is None else service2.request.parameters

information.append({
"List": ["Old list", "New list"],
"list": ["Old list", "New list"],
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
"Values": [f"\\{param_list1}", f"\\{param_list2}"]
})

Expand Down Expand Up @@ -340,22 +341,22 @@ def compare_services(self, service1: DiagService, service2: DiagService) -> list
changed_params += "positive response parameter list, "
# infotext
information.append(
f"List of positive response parameters for service '{service2.short_name}' is not identical."
f"list of positive response parameters for service '{service2.short_name}' is not identical."
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
)
# table
information.append({
"List": ["Old list", "New list"],
"list": ["Old list", "New list"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"list": ["Old list", "New list"],
"List": ["Old list", "New list"],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @kakoeh

Thanks for the input, and I have removed all these lines of code as they differ from the requirement.

Thanks
Vinoth

"Values": [str(response1.parameters),
str(response2.parameters)]
})
else:
changed_params += "positive responses list, "
# infotext
information.append(
f"List of positive responses for service '{service2.short_name}' is not identical.")
f"list of positive responses for service '{service2.short_name}' is not identical.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"list of positive responses for service '{service2.short_name}' is not identical.")
f"List of positive responses for service '{service2.short_name}' is not identical.")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @kakoeh

Thanks for the input, and I have removed all these lines of code as they differ from the requirement.

Thanks
Vinoth

# table
information.append({
"List": ["Old list", "New list"],
"list": ["Old list", "New list"],
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
"Values": [str(service1.positive_responses),
str(service2.positive_responses)]
})
Expand All @@ -382,23 +383,23 @@ def compare_services(self, service1: DiagService, service2: DiagService) -> list
changed_params += "positive response parameter list, "
# infotext
information.append(
f"List of positive response parameters for service '{service2.short_name}' is not identical.\n"
f"list of positive response parameters for service '{service2.short_name}' is not identical.\n"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"list of positive response parameters for service '{service2.short_name}' is not identical.\n"
f"List of positive response parameters for service '{service2.short_name}' is not identical.\n"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @kakoeh

Thanks for the input, and I have removed all these lines of code as they differ from the requirement.

Thanks
Vinoth

)
# table
information.append({
"List": ["Old list", "New list"],
"list": ["Old list", "New list"],
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
"Values": [str(response1.parameters),
str(response2.parameters)]
})
else:
changed_params += "negative responses list, "
# infotext
information.append(
f"List of positive responses for service '{service2.short_name}' is not identical.\n"
f"list of positive responses for service '{service2.short_name}' is not identical.\n"
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
)
# table
information.append({
"List": ["Old list", "New list"],
"list": ["Old list", "New list"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"list": ["Old list", "New list"],
"List": ["Old list", "New list"],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @kakoeh

Thanks for the input, and I have removed all these lines of code as they differ from the requirement.

Thanks
Vinoth

"Values": [str(service1.negative_responses),
str(service2.negative_responses)]
})
Expand Down Expand Up @@ -584,12 +585,29 @@ def add_subparser(subparsers: SubparsersList) -> None:
required=False,
help="Don't show all service parameter details",
)

# TODO
# Idea: provide folder with multiple pdx files as argument
# -> load all pdx files in folder, sort them alphabetically, compare databases pairwaise
# -> calculate metrics (number of added services, number of changed services, number of removed services, total number of services per ecu variant, ...)
# -> display metrics graphically
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
parser.add_argument(
"-f",
"--folder",
#action="store_false",
#default=True,
metavar="FOLDER",
required=False,
help="Provide folder path containing multiple PDX files for pairwise comparison.",
)
parser.add_argument(
"-o",
"--output",
#action="store_false",
#default=True,
metavar="RESULT",
required=False,
help="Write comparison results to JSON file.",
)


def run(args: argparse.Namespace) -> None:
Expand All @@ -598,7 +616,6 @@ def run(args: argparse.Namespace) -> None:
task.param_detailed = args.no_details

db_names = [args.pdx_file if isinstance(args.pdx_file, str) else str(args.pdx_file[0])]

if args.database and args.variants:
# compare specified databases, consider only specified variants

Expand Down Expand Up @@ -693,7 +710,7 @@ def run(args: argparse.Namespace) -> None:
dl for db in task.databases for dl in db.diag_layers
if dl.short_name in task.diagnostic_layer_names
]

# print("NAMES: ",task.diagnostic_layer_names)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dead code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @andlaus
Removed the dead code.
Thanks
Vinoth

for name in args.variants:
if name not in task.diagnostic_layer_names:
rich_print(f"The variant '{name}' could not be found!")
Expand All @@ -714,7 +731,90 @@ def run(args: argparse.Namespace) -> None:
)
task.print_dl_changes(
task.compare_diagnostic_layers(dl, task.diagnostic_layers[db_idx + 1]))

else:
# no databases & no variants specified
rich_print("Please specify either a database or variant for a comparison")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd keep the else block at the end of the if / elif statement to catch all commands where neither -db, -v nor -f were specified.
Suggestion:

    else:
        # no databases, variants or folder specified
        rich_print("Please specify either a database (-db DATABASE [DATABASE ...]), variant (-v VARIANT [VARIANT ...]) or folder (-f FOLDER) for a comparison")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still, no information is printed if neither -db, -v nor -f were specified. To catch all combinations of optional arguments and reject superfluous ones, I suggest following structure of the if else block:

if args.database:

        if args.folder: 
            rich_print("Both options '-db' and '-f' were specified. Please select one of these options.")

        if args.variants:
            # filter considered diagnostic layers (task.diagnostic_layer_names)

        # compare specified databases

elif args.folder:

        if args.variants:
            # filter considered diagnostic layers (task.diagnostic_layer_names)

        # compare pdx files in folder
        
elif args.variants:
        # no databases or folder specified 
        # -> comparison of diagnostic layers in args.pdx_file

else:
        # no databases, variants or folder specified
        rich_print("Please specify either a database (-db DATABASE [DATABASE ...]), variant (-v VARIANT [VARIANT ...]) or folder (-f FOLDER) for a comparison")

#elif args.folder:
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
elif hasattr(args, "folder") and args.folder:
Comment thread
vinothk-master marked this conversation as resolved.
Outdated
print("Now printing the pdx files in folder")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
print("Now printing the pdx files in folder")
print(f"PDX files in folder {args.folder}: {','.join(pdx_files)}")

(you need to move this to line 743)

pdx_files = []
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By iterating through folder, the positional argument PDX_FILE is ignored. Initialize pdx_files with db_names to add PDX_FILE to the list of considered databases.

Suggested change
pdx_files = []
pdx_files = db_names

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still, args.pdx_file is not included in the comparison. Since this argument is mandatory, it does not make sense to skip this file, from my point of view.

for file in os.listdir(args.folder):
if file.lower().endswith(".pdx"):
full_path = os.path.join(args.folder, file)
pdx_files.append(full_path)
pdx_files.sort()
summary_results = []
for i in range(len(pdx_files) - 1):
file_a = pdx_files[i]
file_b = pdx_files[i + 1]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this does not really make sense IMO: the desired order of files to be compared pairwise might not be lexicographical. Possibly the whole pdx_files list ought to be specified as command line parameters? (this would also remove the necessity of walking over the specified directory.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @andlaus
Thanks for the suggestion!

Actually as per the comment from @kayoub5,
load all pdx files in folder, sort them alphabetically, compare databases pairwise

this is the condition, should we go with alphabetically or do we have to compare each and every file without sorting ?

Could you please provide me your input?

Thanks
Vinoth Kannan

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is still the opportunity to pass multiple PDX files in a custom order with the command line parameter -db. From my perspective, -f FOLDER would be used when many files exists and / or typing the list of databases is considered too cumbersome. But @andlaus you're right, lexicographical order might not be desired. Assuming that the files in the folder are already sorted as wanted, pdx_files.sort() could be omitted.

task.databases = [load_file(file_a)]
db_a = task.databases[0]
task.databases = [load_file(file_b)]
db_b = task.databases[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

task.databases can store multiple databases as it is of type list[Database]

Suggested change
db_b = task.databases[0]
db_a = load_file(file_a)
db_b = load_file(file_b)
task.databases += [db_a, db_b]

dl_a = db_a.diag_layers
dl_b = db_b.diag_layers
names_a = {dl.short_name for dl in dl_a}
names_b = {dl.short_name for dl in dl_b}

variants_added = names_b - names_a
variants_deleted = names_a - names_b
variants_changed_count = 0
services_changed_set = set()

diagnostic_layer_names = {dl.short_name
for dl in dl_a}.intersection({dl.short_name
for dl in dl_b})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Idea: diagnostic_layer_names could be filtered by variants if given as input argument

Suggested change
if args.variants:
task.diagnostic_layer_names = diagnostic_layer_names.intersection(set(args.variants))
for name in args.variants:
if name not in task.diagnostic_layer_names:
rich_print(f"The variant '{name}' could not be found!")
return

for name in diagnostic_layer_names:
layer_a = next(dl for dl in dl_a if dl.short_name == name)
layer_b = next(dl for dl in dl_b if dl.short_name == name)
changes = task.compare_diagnostic_layers(layer_a, layer_b)
old_names = []
if getattr(changes, "changed_name_of_service", None):
try:
old_names = changes.changed_name_of_service[0]
except Exception:
# handle different shapes safely
#old_names = list(changes.changed_name_of_service)
old_names = [
item for sublist in changes.changed_name_of_service for item in sublist
]

This comment was marked as outdated.

num_new = len(getattr(changes, "new_services", []) or [])
num_deleted = len(getattr(changes, "deleted_services", []) or [])
num_renamed = len(old_names)
num_changed_params = len(
getattr(changes, "changed_parameters_of_service", []) or [])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

new_services, deleted_services, changed_name_of_service and changed_parameters_of_service are mandatory parameters of ServiceDiff, therefore I'd suggest to be typesafe here

Suggested change
getattr(changes, "changed_parameters_of_service", []) or [])
num_new = len(changes.new_services)
num_deleted = len(changes. deleted_services)
num_renamed = len(changes.changed_name_of_service[0])
num_changed_params = len(changes.changed_parameters_of_service)


# collect which services had parameter changes (unique)
for param_detail in getattr(changes, "changed_parameters_of_service", []) or []:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
for param_detail in getattr(changes, "changed_parameters_of_service", []) or []:
for param_detail in changes.changed_parameters_of_service:

# param_detail.service.short_name exists in your output above
services_changed_set.add(param_detail.service.short_name)

# if anything changed in this variant mark it as changed
if num_new or num_deleted or num_renamed or num_changed_params:
variants_changed_count += 1

summary_results.append({
"file_a": os.path.basename(file_a),
"file_b": os.path.basename(file_b),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

base_file and comparison_file might facilitate understanding which file was compared to which

Suggested change
"file_b": os.path.basename(file_b),
"base_file": os.path.basename(file_a),
"comparison_file": os.path.basename(file_b),

"diag_layer": changes.diag_layer,
"diag_layer_type": changes.diag_layer_type,
"num_variants_added": len(variants_added),
"num_variants_changed": variants_changed_count,
"num_variants_deleted": len(variants_deleted),
"num_new_services": len(changes.new_services),
"num_deleted_services": len(changes.deleted_services),
"num_renamed_services": len(changes.changed_name_of_service[0]),
"num_changed_parameters": len(changes.changed_parameters_of_service)
})

This comment was marked as outdated.

services_a = {srv.short_name for srv in layer_a.services}
services_b = {srv.short_name for srv in layer_b.services}
print("Services in file A:", services_a)
print("Services in file B:", services_b)

print("New services:", services_b - services_a)
print("Deleted services:", services_a - services_b)
print_dl_metrics([layer_a, layer_b])

This comment was marked as outdated.


print(json.dumps(summary_results, indent=4))
if args.output:
with open(args.output, "w") as f:
json.dump(summary_results, f, indent=4)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Idea:
If the content of SpecsChangesVariants and ServiceDiff are mapped to the dictionary summary_results in an independent function, the changes could be printed to a json file as well if arguments -db or -v are given.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @kakoeh
I haven't done the json implementation in this merge request. I felt that once the table output is printed as per the requirement we will move to this Json implementation. Could you please let me know will this work ?

Thanks
Vinoth Kannan

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @vinothk-master
Sure, that's a good idea

print_change_metrics(summary_results)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't there already a function which compares two complete PDX files? why not calling that here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @andlaus

I have reused the compare_dianostic_layer() function to compare different pdx files

Suggested change
print_change_metrics(summary_results)
changes = task.compare_diagnostic_layers(layer_a, layer_b)

Thanks
Vinoth

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @vinothk-master,
@andlaus is right, there is already a function which compares 2 PDX files: compare_databases()
Thus, db_changes = task.compare_databases(db_a, db_b) could be used and replace the loop for name in diagnostic_layer_names where the change metrics are calculated per diagnostic layer.
Instead of printing changes for each diagnostic layer individually with print_dl_metrics([layer_a, layer_b]), task.print_database_changes(db_changes) could then be used.

Copy link
Copy Markdown
Contributor Author

@vinothk-master vinothk-master Sep 3, 2025

Choose a reason for hiding this comment

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

Hi @kakoeh,

I have made the changes as mentioned above and included a new function called print_change_metrics to print the table, especially for the services added/changed/deleted.

Thanks
Vinoth

Binary file added pdx_files_folder/somersault.pdx
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @andlaus, based on the added code, there is no necessity to upload the files somersault.pdx, somersault_modified.pdx and somersault_modified_1.pdx. Maybe it's best to add *.pdx to your .gitignore file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe it's best to add *.pdx to your .gitignore file

the problem with that is that .gitignore is version controlled, i.e., it is quite easy to accidentally commit such a change. that said, maybe PDX files in the odxtools source tree ought to be ignored by everyone, i.e. changes to the somersault pdx files would need to be explicitly staged using git add -f? opinions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable

Binary file not shown.
Binary file added pdx_files_folder/somersault_modified.pdx
Binary file not shown.
Binary file added pdx_files_folder/somersault_modified_1.pdx
Binary file not shown.
Loading