Skip to content

Conversation

@marwin1991
Copy link
Member

Implement: #19 (comment)

@marwin1991
Copy link
Member Author

cc @frakman1

@marwin1991 marwin1991 requested a review from angrymeir November 17, 2024 19:04
@marwin1991
Copy link
Member Author

@angrymeir can I ask you for fast code review and merge and release for @frakman1

:)

@marwin1991
Copy link
Member Author

@angrymeir friendly reminder

@angrymeir
Copy link
Collaborator

@marwin1991 Thank you so much for the implementation! Looks good to me :) I'll do one final test run tonight and then we're ready to go 🚀


class Suppression:
def __init__(self, type, value):
def __init__(self, id, type, value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest we make the parameters optional (def __init__(self, id=None, type=None, value=None)) here, since either id or type and value will be empty. And otherwise the old configuration will assign the type to id and the value to type leaving value empty.

Copy link
Collaborator

@angrymeir angrymeir Nov 21, 2024

Choose a reason for hiding this comment

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

Please let me know if that makes sense to you :)

Btw fyi: In the next days I'll integrate the unit tests into our CI, so we can automatically see if there's any issues

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess in addition we'll have to adjust
expected = Config([Suppression('cwe', '2555'), Suppression('find_sec_bugs_type', 'SPRING_ENDPOINT')])

to

expected = Config([Suppression(None, 'cwe', '2555'), Suppression(None, 'find_sec_bugs_type', 'SPRING_ENDPOINT')])

in /tests/test_config.py

Copy link
Member Author

Choose a reason for hiding this comment

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

@angrymeir You are rigth! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@angrymeir fixeds :)

Copy link
Collaborator

@angrymeir angrymeir 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 so much @marwin1991!
I'll push the new release once you merged it :)

@marwin1991 marwin1991 merged commit 6508716 into main Nov 21, 2024
2 checks passed
@marwin1991
Copy link
Member Author

@angrymeir let's go 🚀 🌔

@marwin1991 marwin1991 deleted the suppresions-by-id branch November 21, 2024 16:46
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.

3 participants