Skip to content

Refactor parse_diffoscope_json #23

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
Merged
merged 59 commits into from
Sep 22, 2025
Merged

Refactor parse_diffoscope_json #23

merged 59 commits into from
Sep 22, 2025

Conversation

sei-aderr
Copy link
Contributor

@sei-aderr sei-aderr commented Sep 11, 2025 •
edited
Loading

Refactor of parse_diffoscope_json function. Mainly followed the suggestions from Sebastian from a while ago that I saved in the issue

  • Made the functionality be contained within a class
  • Made a _check_flags function, and made each separate check into its own function
  • Made the recursion a separate function
  • Moved all handling of recursive iteration into the recursion function
  • Renamed _process_and_save_results to _process_and_write_results
  • Renamed is_path to is_abs_path and added tests
  • Removed no longer used parent_source parameters and class variables from Diff

Would appreciate a quick review of how it looks now, to see if there are other changes that should be made before I write unit tests for each sub function if they were to change.

@sei-aderr
_process_and_save > _process_and_write
de06d02
@sei-aderr
Initial working version of Diffoscope Parser
aff36c6 
Doesn't handle file lookup yet. Also, metadata diffs based on metadata.json not yet implemented in previous PR to work here
@sei-aderr
Add filetype checks and the updated _check_flag section
f8d45de
@sei-aderr
Merge branch 'feature/docker-metadata' of https://github.com/cmu-sei/…
f20568c 
…vessel into refactor/parse-diffoscope
@sei-aderr
Add parser to merged diff_command
1b204c4
@sei-aderr
Remove parse_diffoscope and tests. RIP
7654090
@sei-aderr
Make each flag check a function
3cf5c2d
@sei-aderr
Flag always has its fields, don't need default values
ebeecef
@sei-aderr
Have parser execute in init and contain its data instead of return
e7fd684
@sei-aderr
Update function name and add tests
7ddf01b
@sei-aderr
Refactor recursion checks, remove unused diff vars
7dd6db1
@sei-aderr
Remove uneeded initialization
c09486e
@sei-aderr
save > write
229f220
@sei-aderr
Docstrings and QA
0e58bc8
@sei-aderr
Re-order functions based on new call order
20fb4b0
sei-aderr
sei-aderr commented Sep 11, 2025
View reviewed changes
sei-lzhan
sei-lzhan reviewed Sep 12, 2025
View reviewed changes
sei-secheverria
sei-secheverria requested changes Sep 12, 2025
View reviewed changes
Copy link
Contributor

@sei-secheverria sei-secheverria left a comment

Choose a reason for hiding this comment

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

Very good improvements, several comments below

@sei-aderr
Copy link
Contributor Author

Consider renaming plus_line and minus_line variables and any other references to them.

@sei-aderr
Copy link
Contributor Author

Ready for another review. Lihan fixed the other tests in test_diff_command which will be pushed shortly. Once these pieces look good I will flesh out the unit tests for the new code.

@sei-lzhan
Copy link
Contributor

diff_command unit test fixed, ready for review

sei-secheverria
sei-secheverria requested changes Sep 17, 2025
View reviewed changes
Copy link
Contributor

@sei-secheverria sei-secheverria 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, a couple of very minor points

sei-kpitstick
sei-kpitstick approved these changes Sep 22, 2025
View reviewed changes
Copy link
Contributor

@sei-kpitstick sei-kpitstick left a comment

Choose a reason for hiding this comment

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

Tested, results line up.

@sei-kpitstick sei-kpitstick merged commit c2ce6ea into main Sep 22, 2025
2 checks passed
@sei-kpitstick sei-kpitstick deleted the refactor/parse-diffoscope branch September 22, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sei-lzhan sei-lzhan sei-lzhan left review comments

@sei-secheverria sei-secheverria sei-secheverria approved these changes

@sei-kpitstick sei-kpitstick sei-kpitstick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sei-aderr @sei-lzhan @sei-secheverria @sei-kpitstick