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

Allow to add arbitrary values to `PropertyBag` #101

Merged

BraisGabin merged 25 commits into main from fix-78

Feb 27, 2024

Member

BraisGabin commented Feb 13, 2024 •

edited

Fix #78

I would like to get feedback from the API. I don't like too much that PropertyBag is now a Map<String, JsonElement> because that couples us with kotlinx.serialization. But I don't see other way to implement this:

~~The property values MAY be of any JSON type, including strings, numbers, arrays, objects, Booleans, and null. If a property value is a string, it MAY be an empty string.~~

~~(Extracted from the specification: https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/sarif-v2.1.0-errata01-os-complete.html#_Toc141790698)~~

~~Any feedback is more than welcome~~

I was facing this issue: Kotlin/kotlinx.serialization#296 so I needed to create those chunky functions. They are not bullet proof but they should do the job.

BraisGabin mentioned this pull request

PropertyBag appears to be implemented different from its doc #78

Closed

BraisGabin marked this pull request as draft

February 13, 2024 16:29

BraisGabin requested review from chao2zhang and removed request for chao2zhang

February 13, 2024 18:25

BraisGabin marked this pull request as ready for review

February 13, 2024 18:26

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/Merging.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/jvmTest/resources/test.sarif.json Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/jvmTest/resources/test.sarif.json Show resolved Hide resolved

BraisGabin added 2 commits

February 14, 2024 00:41


          Implement PropertyBag properly

2a908c0


          Don't expose JsonElement

ad749e9

BraisGabin force-pushed the fix-78 branch from 3b51d8c to ad749e9 Compare

February 13, 2024 23:41


          Update src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt Outdated Show resolved Hide resolved

BraisGabin and others added 8 commits

February 14, 2024 13:12


          Update src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt

0da43f8

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>


          Update src/commonMain/kotlin/io/github/detekt/sarif4k/SarifSerializer.kt

3acbeca

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>


          Move code to its own file

aa8184f


          Improve error messages

8d60533


          Use companion object to follow the convention in the rest of the file

62a7ea8


          move Arrays down

99f1664

WIP

a263b46


          Add more tests

fa1b557

BraisGabin force-pushed the fix-78 branch from ae3474f to fa1b557 Compare

February 22, 2024 09:10

Member Author

BraisGabin commented Feb 22, 2024

Ok, I think that I fixed all the comments. Thanks for the reviews! And with the test that I added in the last commit it feels more secure.


          Merge branch 'main' into fix-78

9b5f8b2

TWiStErRob reviewed

View reviewed changes

Member

TWiStErRob left a comment

Sorry, I guess I looked deeper now than previously? 😅

Note: I did the review on my phone because I'm sick in bed, so there might be some weird comments. 😵‍💫

src/commonMain/kotlin/io/github/detekt/sarif4k/Merging.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/JsonElementExtensions.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/JsonElementExtensions.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/JsonElementExtensions.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/JsonElementExtensions.kt Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

src/jvmTest/kotlin/io/github/detekt/sarif4k/PropertyBagTest.kt Outdated Show resolved Hide resolved

src/jvmTest/kotlin/io/github/detekt/sarif4k/PropertyBagTest.kt Outdated Show resolved Hide resolved

BraisGabin added 8 commits

February 23, 2024 09:09


          Reduce visibility

400e08c


          Rename

a94dcef


          format

589a2a2


          Add missing test

274ab50


          format

05701ea


          format

9775c15


          Simplify code


          Improve exception messages

dd3a05f

Member Author

BraisGabin commented Feb 23, 2024

Sorry, I guess I looked deeper now than previously? 😅

No need to apology! This review was great :)

TWiStErRob reviewed

View reviewed changes

src/jvmTest/kotlin/io/github/detekt/sarif4k/PropertyBagTest.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Outdated Show resolved Hide resolved

src/commonMain/kotlin/io/github/detekt/sarif4k/JsonElementExtensions.kt Outdated Show resolved Hide resolved

BraisGabin and others added 4 commits

February 27, 2024 00:06


          Update src/commonMain/kotlin/io/github/detekt/sarif4k/JsonElementExte…

0bec44d

…nsions.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>


          Update src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindin…

da90097

…gs.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>


          Update src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindin…

577564f

…gs.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>


          Add corrections

e6d1b83

Member Author

BraisGabin commented Feb 26, 2024

Ok, I think that now it's ready

TWiStErRob reviewed

View reviewed changes

src/commonMain/kotlin/io/github/detekt/sarif4k/SarifDataBindings.kt Show resolved Hide resolved

TWiStErRob approved these changes

View reviewed changes


          Add comment

534d89e

BraisGabin enabled auto-merge (squash)

February 27, 2024 11:25

BraisGabin merged commit 590a17e into main

2 checks passed

BraisGabin deleted the fix-78 branch

February 27, 2024 11:28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment