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

Add endColumn/endLine to SARIF region #5011

Merged
merged 4 commits into from Jul 1, 2022

Conversation

VitalyVPinchuk
Copy link
Contributor

For #4919

Looks like the following:

SARIF finding image

But I wasn't able to come up with good tests so I just compare result against predefined text.

@cortinico
Copy link
Member

This is great. @chao2zhang do you want to pick this up?

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I'm far from a computer for some days so I can't check. But isn't the psi api give us this information the same way it give us the start? I'm not 100% about this, I couldn't check the code that we have right now.


every { psiFileMock.text } returns code
every { ktElementMock.containingFile } returns psiFileMock
return ktElementMock
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid the usage of mockk here if possible. Could this ktelement created/faked instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.
But why is it a bad idea to use mockk here?

Replace toSet() with distinct(), get rid of null check.
@VitalyVPinchuk
Copy link
Contributor Author

But isn't the psi api give us this information the same way it give us the start?

Sorry, could you be more specific?

Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

Thank you for adding tests!

@BraisGabin
Copy link
Member

Sorry, could you be more specific?

Chao answer this already. We need to refactor core to make this more precise. But that's out of scope of this PR.

@chao2zhang chao2zhang merged commit b49aea7 into detekt:main Jul 1, 2022
@BraisGabin
Copy link
Member

@VitalyVPinchuk first of all thank you so much for all the contributions they are really good <3

I want to ask you a favor. In next PRs, if you feel that you are fixing an issue use the ver 'close' or 'fix' before the number or the issue. This way gothub will be automatically close the issue once we merge the PR. This is not a big deal at all but if you can do that it will help a lot so we don't keep already solved issues open :)

@VitalyVPinchuk
Copy link
Contributor Author

In next PRs, if you feel that you are fixing an issue use the ver 'close' or 'fix' before the number or the issue. This way gothub will be automatically close the issue once we merge the PR.

Yes, sure! Thanks for letting me know.

@VitalyVPinchuk VitalyVPinchuk deleted the sarif_end_block_fix branch July 2, 2022 19:05
@cortinico cortinico added this to the 1.21.0 milestone Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants