Skip to content

feat: Add support for --sarif flag to qlty smells command #2037

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

Merged
merged 10 commits into from
Jun 2, 2025

Conversation

ujlbu4
Copy link
Contributor

@ujlbu4 ujlbu4 commented May 8, 2025

This PR adds support for generating SARIF output using the --sarif flag when running the qlty smells command.


Notes:

I'm not an expert in Rust, and I encountered a few challenges while implementing this feature, so happy for any comments to improve it.

The main issue was that the existing SarifFormatter only supported a single type: qlty_check::Report. Since Rust doesn't have inheritance, I resolved this by extracting a shared trait (SarifTrait) and implementing it for both qlty_check::Report and qlty_analysis::Report. The SarifFormatter was then updated to accept any type that implements this trait.

I'm not completely confident that all the structs and traits are placed in the most appropriate modules, so I'm open to feedback or suggestions about how to better organize them according to project conventions or best practices.

@CLAassistant
Copy link

CLAassistant commented May 8, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

qltysh bot commented May 8, 2025

All good ✅

Copy link
Member

@brynary brynary left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

I agree there's no good reason that qlty check supports --sarif but qlty smells does not, so I think this is a good feature.

In the future, we are going to be extracting the maintainability analysis that is in qlty smells and qlty metrics into a separate crate so that it can integrate more cleanly. This will have then effect of avoiding these kinds of issues. But the timeline for that is not set yet and this is helpful in the meantime.

As far as the design goes, I would try and avoid introducing a trait. What if we update the SarifFormatter struct so that instead of storing a Report as its fields it instead stores Vec<Message> and Vec<Issue>?

@ujlbu4
Copy link
Contributor Author

ujlbu4 commented May 13, 2025

Hey @brynary, thanks for the suggestion! It really makes sense and actually simplifies the code. 🙂

Not sure why I didn’t go that route initially, probably didn’t want to touch the original interfaces. Anyway, I’ve pushed your suggestions. Hope it looks better now! )

@ujlbu4
Copy link
Contributor Author

ujlbu4 commented May 14, 2025

hopefully fixed tests, added missed import

@ujlbu4 ujlbu4 force-pushed the feat-add-sarif-support-for-smells branch from a90f180 to 2de2710 Compare May 21, 2025 23:01
@ujlbu4 ujlbu4 force-pushed the feat-add-sarif-support-for-smells branch from 2de2710 to 8afeb9c Compare May 21, 2025 23:02
Copy link
Member

@brynary brynary left a comment

Choose a reason for hiding this comment

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

This looks great! Will merge shortly.

@brynary brynary merged commit d0b3888 into qltysh:main Jun 2, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants