Skip to content

Conversation

kingthorin
Copy link
Contributor

Fixes #21.

Since there was no further input I decided I was right 👼

@kingthorin kingthorin requested a review from arkid15r as a code owner October 20, 2025 22:40
@kingthorin

This comment was marked as outdated.

@kingthorin

This comment was marked as outdated.

@arkid15r
Copy link
Collaborator

Hey @kingthorin thanks for addressing this! Sorry, I'm busy with other stuff recently -- I'll review this tomorrow.

@arkid15r
Copy link
Collaborator

@rudransh-shrivastava wanna try reviewing some code, I think you're familiar enough with the code base. No pressure, it's an optional one.

@rudransh-shrivastava
Copy link
Collaborator

@arkid15r sure!

Copy link
Collaborator

@rudransh-shrivastava rudransh-shrivastava left a comment

Choose a reason for hiding this comment

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

Looks good to me!
I think the positive test required_properties.yaml can be renamed, it sounds a little misleading now. We can also introduce 3 positive tests like large_only.yaml/large_property.yaml.
I'll add these tests in a new PR after this is merged.

Just a small change:

@kingthorin
Copy link
Contributor Author

@rudransh-shrivastava great point on the tests. I’m happy to add those (and address the sorting comment).

I’ll update this shortly.

@rudransh-shrivastava
Copy link
Collaborator

@kingthorin thank you

@kingthorin kingthorin force-pushed the logo-anyof branch 3 times, most recently from 30ee7e5 to d2d4e99 Compare October 22, 2025 12:58
Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@kingthorin
Copy link
Contributor Author

I think this is good to go now.

Let me know if you'd like the .gitignore additions in a separate PR. (.DS_Store because I'm working on a mac, .project because I did some of the work in Eclipse IDE).

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @kingthorin

@rudransh-shrivastava appreciate your help with the review

@arkid15r arkid15r merged commit 9169aef into OWASP:main Oct 23, 2025
6 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.

Revisit logo requirements

3 participants