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: implement sarif format reporter #1047

Merged
merged 1 commit into from
Dec 16, 2024
Merged

feat: implement sarif format reporter #1047

merged 1 commit into from
Dec 16, 2024

Conversation

Zxilly
Copy link
Contributor

@Zxilly Zxilly commented Jun 28, 2024

Closes: #594

I asked earlier if anyone had already worked on this feature and got no reply #594 (comment) , so I added this implementation myself.

But as I declared, I'm not a professional rust developer, so this patch obviously contains a lot of problems caused by a lack of understanding of rust, such as .unwrap abuse, since I'm not sure how to properly convert Error to io::Error.

I enabled Allow edits by maintainers, so if you have any suggestions for syntax changes, please push them directly to the corresponding repository branch, thanks!

crates/typos-cli/Cargo.toml Outdated Show resolved Hide resolved
@Zxilly Zxilly force-pushed the sarif branch 3 times, most recently from c22c553 to 50550f1 Compare June 29, 2024 10:51
@Zxilly Zxilly requested a review from epage June 29, 2024 10:51
@Zxilly
Copy link
Contributor Author

Zxilly commented Jun 29, 2024

@epage Could you please approve the workflow run to see if anything broken by this?

@coveralls
Copy link

coveralls commented Jun 29, 2024

Pull Request Test Coverage Report for Build 9723606862

Details

  • 0 of 76 (0.0%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.7%) to 22.315%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/typos-cli/src/bin/typos-cli/args.rs 0 1 0.0%
crates/typos-cli/src/bin/typos-cli/main.rs 0 1 0.0%
crates/typos-cli/src/report.rs 0 2 0.0%
crates/typos-cli/src/bin/typos-cli/report.rs 0 72 0.0%
Files with Coverage Reduction New Missed Lines %
crates/typos-cli/src/bin/typos-cli/main.rs 2 0.0%
Totals Coverage Status
Change from base Build 9627078583: -0.7%
Covered Lines: 534
Relevant Lines: 2393

💛 - Coveralls

@Zxilly
Copy link
Contributor Author

Zxilly commented Nov 22, 2024

@epage I have resolved the conflict.

@epage
Copy link
Collaborator

epage commented Nov 22, 2024

Sorry for the delays as I am slowly working through my backlog.

It would help if the commits were cleaned up to present this how you want it reviewed and merged.

It would also be good for us to have a test for this, likely among our end-to-end tests.

@Zxilly
Copy link
Contributor Author

Zxilly commented Nov 23, 2024

linter warning resolved

@Zxilly
Copy link
Contributor Author

Zxilly commented Nov 23, 2024

I suppose I could write some integration tests, but I'm not quite sure where the old tests are located, is there any architecture I could reuse?

@Zxilly
Copy link
Contributor Author

Zxilly commented Nov 23, 2024

I have added a test

@epage
Copy link
Collaborator

epage commented Dec 13, 2024

Sorry that I've fallen behind and not been able to get back to this yet.

I did want to quickly note that I just noticed the fix element
https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/sarif-v2.1.0-errata01-os-complete.html#_Toc141791131

I wonder if github has integration for that so people get diffs in the post with the possibility of making a commit right from the review.

Note: I would not block merging this PR on getting that to work

@epage epage force-pushed the sarif branch 2 times, most recently from b6a807e to 9b227c8 Compare December 16, 2024 19:01
@epage
Copy link
Collaborator

epage commented Dec 16, 2024

  • Switched to code points
  • Added more tests
  • Improve handling of file name typos

@epage epage force-pushed the sarif branch 7 times, most recently from 311679f to 6390844 Compare December 16, 2024 20:39
@epage
Copy link
Collaborator

epage commented Dec 16, 2024

@Zxilly mind doing a pass over of my changes?

@Zxilly
Copy link
Contributor Author

Zxilly commented Dec 16, 2024

Look good to me, maybe we can add some cjk char to testcase?

@epage epage merged commit 370109d into crate-ci:master Dec 16, 2024
21 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.

Support Sarif reporting format
3 participants