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.

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

[feat] Single plist #4364

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[feat] Single plist #4364

wants to merge 8 commits into from

Conversation

vodorok
Copy link
Contributor

@vodorok vodorok commented Oct 14, 2024

This patch adds the logic of report output merging into one singular .plist file. Now analyze and check can be invoked with the --plist-file-name option, which if provided will merge all new.plist result files into the specified file name under the specified output report folder.

@vodorok vodorok requested a review from bruntib as a code owner October 14, 2024 12:56
@vodorok vodorok changed the title Single plist [feat] Single plist Oct 14, 2024
@vodorok vodorok self-assigned this Oct 14, 2024
@vodorok vodorok added enhancement 🌟 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands labels Oct 14, 2024
@vodorok vodorok force-pushed the single_plist branch 3 times, most recently from 90535b3 to 651f925 Compare October 14, 2024 14:31
@dkrupp dkrupp added this to the release 6.25.0 milestone Nov 5, 2024
This patch adds the logic of report output merging into one singluar
`.plist` file. Now analyze and check can be invoked with the
`--plist-file-name` option, which if provided will merge all new
`.plist` result files into the specified file name under the specified
output report folder.
Comment on lines +63 to +67
def merge_plists(results, output_path, plist_file_name):
"""
Merge the plist files generated by the analyzers into a single plist file.
Deletes the original plist files after merging.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we are not reusing the method introduced in #4152? That also merged multiple plist files, while also uniquing bug report with the same hash (as generated by get_report_path_hash(), not by the analyzer).

Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Generally speaking, I'm highly against the fixation on a specific output format. Sure, plist is the format we use, and will likely use way past you and I bite the dust, but its still distracting. I prefer consistently naming user-facing configs like "output-single-file" instead of "plist-single-file".

On another issue, as I stated in my previous response, there is no reason not to unique the reports in the unified plist file, for which we already implemented a solution. Having the same thing be implemented twice needs more justification.

Also, I'm missing a great dose of "why" in the summary. Why do we need this feature? I understand that this in part to better support bazel projects. Why is it needed/advantegeous to generate a single plist files for them? These kinds of details should be most highlighted in the summary.

edit: also, I don't think the PR name has a character limit! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands enhancement 🌟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants