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.

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

Issue 252 #259

Merged
merged 2 commits into from
Dec 9, 2023
Merged

Issue 252 #259

merged 2 commits into from
Dec 9, 2023

Conversation

timbray
Copy link
Owner

@timbray timbray commented Dec 8, 2023

The problem was that {"data": {"field": []}}, which is syntactically valid, would make addPattern blow up. Which at one level is sane because it can't match anything, but on the other hand if you're generating Patterns it would be easy to write code to "match any of the elements of my array PartNumbers" and if PartNumbers was empty you'd get this kind of pattern and while it couldn't match anything, that would be the expected correct behavior.

The reason for excluding that particular message in the gosec linter is that the linter wants the crypto/rand as opposed to math/rand, but we want to retain the use of a fixed seed for the random number generator for testing/debugging purposes, and crypto/rand doesn't seem to support that and also is slower.

ignore fields with no values

Signed-off-by: Tim Bray <[email protected]>
Signed-off-by: Tim Bray <[email protected]>
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (25e8bd7) 96.19% compared to head (8b58111) 96.19%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #259   +/-   ##
=======================================
  Coverage   96.19%   96.19%           
=======================================
  Files          17       17           
  Lines        2126     2129    +3     
=======================================
+ Hits         2045     2048    +3     
  Misses         54       54           
  Partials       27       27           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timbray timbray requested a review from embano1 December 8, 2023 23:21
Copy link
Collaborator

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

One nit/question.

Also, can you please update your PR description. Currently it only contains the linter issue but this PR contains also the fix for empty array.

checkXEqual(t, wanted[want], matches)
}
}
func checkXEqual(t *testing.T, x1s []X, x2s []X) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I just wonder whether slices.Sort and then slices.Equal from https://pkg.go.dev/golang.org/x/exp/slices would be more clear instead of creating a new equality check helper?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good idea. Unfortunately the "X" type doesn't implement cmp.ordered.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated PR

@timbray timbray merged commit adde419 into main Dec 9, 2023
8 checks passed
@timbray timbray deleted the issue-252 branch December 9, 2023 19:09
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