Skip to content

Removing redundant TryGetNonEmptyValue when key is required#539

Merged
eddynaka merged 3 commits into
mainfrom
users/ednakamu/removing-useless-tryget
Aug 15, 2021
Merged

Removing redundant TryGetNonEmptyValue when key is required#539
eddynaka merged 3 commits into
mainfrom
users/ednakamu/removing-useless-tryget

Conversation

@eddynaka

@eddynaka eddynaka commented Aug 15, 2021

Copy link
Copy Markdown
Collaborator

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • ReleaseHistory.md updated for non-trivial changes
  • Added unit tests

@michaelcfanning

michaelcfanning commented Aug 15, 2021

Copy link
Copy Markdown
Member

Eddy, where is the release note describing this change? No more changes of this kind without documenting them in the release notes, please. :)

Note that this change is actually key to document as we will now aggressively raise exceptions in cases where we previously did not!


In reply to: 899106233

@michaelcfanning

michaelcfanning commented Aug 15, 2021

Copy link
Copy Markdown
Member
                    Verify(fingerprintText == $"[pat/vs={testCase.Input}]", title, failedTestCases);

this shouldn't work! this is a very old fingerprint. does this test need updating?


In reply to: 899106459


Refers to: Src/Plugins/Tests.Security/Validators/SEC101_102.AdoPatValidatorTests.cs:76 in c0b866e. [](commit_id = c0b866e, deletion_comment = False)

@michaelcfanning michaelcfanning left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🕐

string failureLevel = testCase.FailureLevel;
string fingerprintText = null;
var groups = new Dictionary<string, FlexMatch>();
var groups = new Dictionary<string, FlexMatch>

@michaelcfanning michaelcfanning Aug 15, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

groups

why didn't we notice this problem before? looks like we're perhaps missing a key unit test somewhere? #Resolved

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was a strange test.
I removed a few things and started validating the expected count and state.

@eddynaka

Copy link
Copy Markdown
Collaborator Author
                    Verify(fingerprintText == $"[pat/vs={testCase.Input}]", title, failedTestCases);

This isn't needed. just removed.


In reply to: 899106459


Refers to: Src/Plugins/Tests.Security/Validators/SEC101_102.AdoPatValidatorTests.cs:76 in c0b866e. [](commit_id = c0b866e, deletion_comment = False)

@eddynaka

Copy link
Copy Markdown
Collaborator Author

Added a new entry


In reply to: 899106233

Comment thread Src/ReleaseHistory.md Outdated

## Unreleased

- Plugin Improvement: Required properties will throw exception if it does not exist. [#539](https://github.com/microsoft/sarif-pattern-matcher/pull/539)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it does

'if they do not exist'.

@michaelcfanning michaelcfanning left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@eddynaka eddynaka merged commit ab7fcf0 into main Aug 15, 2021
@eddynaka eddynaka deleted the users/ednakamu/removing-useless-tryget branch August 15, 2021 23:33
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.

2 participants