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.

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

SarifFixAdapter #4

Merged
merged 10 commits into from
Dec 19, 2022
Merged

SarifFixAdapter #4

merged 10 commits into from
Dec 19, 2022

Conversation

kgevorkyan
Copy link
Member

@kgevorkyan kgevorkyan commented Dec 5, 2022 •
edited
Loading

What's done:

  • Introduce primary logic and tests
  • Apply fix objects from SARIF to the test files
  • Improve algorithm and expand tests

@kgevorkyan
Introduce primary logic and tests (#3)
15c5697 
### What's done:
* Introduce primary logic and tests
@kgevorkyan kgevorkyan changed the title Introduce primary logic and tests (#3) SarifFixAdapter (WIP) (#3) Dec 5, 2022
@kgevorkyan
Apply fix objects from SARIF to the test files (#5)
fbd7661 
### What's done:
* Apply fix objects from SARIF to the test files
@kgevorkyan
Algorithm improvements and more tests (#6)
de88318 
### What's done:
* Algorithm improvements and more tests
@kgevorkyan
Improve algorithm and expand tests (#7)
cd1d693 
### What's done:
* Improve algorithm and expand tests
@kgevorkyan
Polishing (#9)
3dce90e 
### What's done:
* Polishing
@kgevorkyan kgevorkyan changed the title SarifFixAdapter (WIP) (#3) SarifFixAdapter Dec 14, 2022
@kgevorkyan kgevorkyan requested review from nulls and petertrr and removed request for nulls and petertrr December 14, 2022 09:28
@kgevorkyan kgevorkyan requested review from petertrr and removed request for nulls December 14, 2022 09:40
nulls
nulls reviewed Dec 14, 2022
View reviewed changes
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt Show resolved Hide resolved
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt Outdated Show resolved Hide resolved
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt Outdated Show resolved Hide resolved
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt Outdated Show resolved Hide resolved
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt Show resolved Hide resolved
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/files/FileUtils.kt Outdated Show resolved Hide resolved
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/files/FileUtils.kt Outdated Show resolved Hide resolved
@kgevorkyan
Enhacements in core logic (#10)
2c582be 
### What's done:
* Сonsider `uriBaseId` for file paths in Sarif
* Add tests
* Polishing
@kgevorkyan
Enhance logging (#14)
37c24f9 
### What's done:
* Enhance logging
@kgevorkyan
Support replacements of whole line
a18d09c 
### What's done:
* Support replacements of whole line
orchestr7
orchestr7 reviewed Dec 19, 2022
View reviewed changes
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt
val currentArtifactLocation = artifactChange.artifactLocation
if (currentArtifactLocation.uri == null) {
println("Error: Field `uri` is absent in `artifactLocation`! Ignore this artifact change")
null
Copy link
Member

Choose a reason for hiding this comment

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

why do you need to store null here? Just filter out objects that have empty uri

Copy link
Member Author

@kgevorkyan kgevorkyan Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's placed inside mapNotNull in if-else condition, so any branch should return something, here we need to log such cases, that's why I separate it. null will be filtered by mapNotNull

orchestr7
orchestr7 reviewed Dec 19, 2022
View reviewed changes
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt Outdated
fs.canonicalize(it) == fullPathOfFileFromSarif
}
if (testFile == null) {
println("Couldn't find appropriate test file on the path ${fileReplacements.filePath}, which provided in Sarif!")
Copy link
Member

Choose a reason for hiding this comment

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

test files? what is the relation of test files to a separate sarif library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, changed to targetFiles

orchestr7
orchestr7 reviewed Dec 19, 2022
View reviewed changes
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt
* @param ruleReplacements list of replacements by all rules
* @return filtered list of replacements by all rules
*/
private fun filterRuleReplacements(ruleReplacements: RuleReplacements): RuleReplacements {
Copy link
Member

@orchestr7 orchestr7 Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should be changed completely with the algorithm stackoverflow proposed: check min and max in the interval.
If overlap - take first

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, will be done under #13 in separate PR

orchestr7
orchestr7 reviewed Dec 19, 2022
View reviewed changes
fixpatches/src/commonMain/kotlin/com/saveourtool/sarifutils/cli/adapter/SarifFixAdapter.kt
* @param sarifFile path to the sarif file with fix object replacements
* @param testFiles list of the test file, to which above fixes need to be applied
*/
class SarifFixAdapter(
Copy link
Member

Choose a reason for hiding this comment

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

why is it an adapter?

And also why do we need "testFiles"?

Forget word 'test'. It is targetFile or something like this

Copy link
Member Author

Choose a reason for hiding this comment

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

I name this by analogy from save-cli, like we have SarifWarnAdapter
Can't imagine better name for now, so leave as is. Will change in future PR

test -> target changed

@kgevorkyan kgevorkyan merged commit 999c609 into main Dec 19, 2022
@kgevorkyan kgevorkyan deleted the dev branch December 19, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orchestr7 orchestr7 orchestr7 left review comments

@nulls nulls nulls left review comments

@0x6675636b796f75676974687562 0x6675636b796f75676974687562 0x6675636b796f75676974687562 approved these changes

@petertrr petertrr petertrr approved these changes

Assignees

@kgevorkyan kgevorkyan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kgevorkyan @nulls @petertrr @orchestr7 @0x6675636b796f75676974687562