-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
updated the script to consider versioned paths #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An open issue is that the px4_msgs are not up-to-date with the version field in the message definition, and thereby the check fails.
There, the question would be if that field will also be present in the px4_msgs repo, or should we just exclude this in the compensation? @bkueng
Are you sure it's causing a failed check? I'm asking because constants should explicitly be excluded here already:
if constant == "=": |
@@ -114,12 +114,19 @@ def compare_files(file1: str, file2: str): | |||
with open(file1, 'r') as f1, open(file2, 'r') as f2: | |||
diff = list(difflib.unified_diff(f1.readlines(), f2.readlines(), fromfile=file1, tofile=file2)) | |||
if diff: | |||
print(f"Mismatch found between {file1} and {file2}:") | |||
print(f"Mismatch found between {file1} and {file2}: ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wasnt able to commit the change without because of the formatting requirements from the precommit hook.
9cc6539
to
03d04ee
Compare
you are right, the version is excluded in the check, i updated the scripts so that they now work. there was an open issue with the path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks
Currently, the script always failed because it can't find the files inside the versioned message folder, with this commit, I'm searching all the subfolders in the
msg
directory, to find the files. Thereby, messages in the versioned folder are also found.An open issue is that the
px4_msgs
are not up-to-date with the version field in the message definition, and thereby the check fails.There, the question would be if that field will also be present in the px4_msgs repo, or should we just exclude this in the compensation? @bkueng