Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Removal of the historical .txt trace file format and related scripts #777

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Removal of the historical .txt trace file format and related scripts #777

merged 5 commits into from
Feb 26, 2024

Conversation

PhRosenberger
Copy link
Contributor

@PhRosenberger PhRosenberger commented Feb 14, 2024 •
edited
Loading

Reference to a related issue in the repository

This PR fixes #760

Add a description

Add a description of the changes proposed in the pull request.

Some questions to ask:
The historical .txt output format for hybrid binary/human readable OSI trace files does not really help anyone and was not used by the community so far. I therefore strongly propose to get rid of it and the related python script for conversion to the actually binary .osi format.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

@PhRosenberger PhRosenberger requested a review from pmai February 14, 2024 17:02
@PhRosenberger PhRosenberger added the Documentation Everything which impacts the quality of the documentation and guidelines. label Feb 14, 2024
@PhRosenberger PhRosenberger self-assigned this Feb 14, 2024
@PhRosenberger PhRosenberger added this to the V3.7.0 milestone Feb 14, 2024
@PhRosenberger PhRosenberger mentioned this pull request Feb 14, 2024
Closed
@PhRosenberger PhRosenberger added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Feb 15, 2024
@thomassedlmayer
Copy link
Contributor

We should probably also remove the format choice of the osi2read argparser and the respective code in the OSITrace.py on the separated txt format.

@PhRosenberger
Copy link
Contributor Author

We should probably also remove the format choice of the osi2read argparser and the respective code in the OSITrace.py on the separated txt format.

I just updated the PR accordingly. Could you please check / test it?

@PhRosenberger
Copy link
Contributor Author

@pmai obviously, removing the format command line argument interfers with our current pypeline. Should we change the test of the python scripts in this PR, as well?

@PhRosenberger
Copy link
Contributor Author

@pmai obviously, removing the format command line argument interfers with our current pypeline. Should we change the test of the python scripts in this PR, as well?

Ok, seems like, I just missed one line, pipeline has passed now.
However, would appreciate some double-check here.

pmai
pmai reviewed Feb 26, 2024
View reviewed changes
Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

Looks good to me, with maybe the additional acknowledgement.

doc/architecture/trace_file_formats.adoc Show resolved Hide resolved
@pmai
Copy link
Contributor

pmai commented Feb 26, 2024

CCB 2024-02-26: Merge with suggested addition of historical note; thanks for the effort.

@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Feb 26, 2024
@thomassedlmayer
Copy link
Contributor

Sorry for the late check. I found another format_type appearance in the file in the osi2read.py.

@pmai pmai force-pushed the 760-bug-in-osi-docu-on-tracefiles branch from af500da to f38c922 Compare February 26, 2024 10:52
@pmai pmai merged commit 77587cf into master Feb 26, 2024
6 checks passed
@pmai pmai deleted the 760-bug-in-osi-docu-on-tracefiles branch February 26, 2024 11:01
jdsika
jdsika reviewed Apr 23, 2024
View reviewed changes
Copy link
Contributor

@jdsika jdsika left a comment

Choose a reason for hiding this comment

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

Reviewed for v3.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdsika jdsika jdsika left review comments

@pmai pmai pmai left review comments

@ClemensLinnhoff ClemensLinnhoff Awaiting requested review from ClemensLinnhoff

Assignees

@PhRosenberger PhRosenberger

Labels
Documentation Everything which impacts the quality of the documentation and guidelines. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Milestone
V3.7.0
Development

Successfully merging this pull request may close these issues.

Bug in OSI Docu on Tracefiles
5 participants
@PhRosenberger @thomassedlmayer @pmai @jdsika @ClemensLinnhoff