davidmarkclements / fast-redact

very fast object redaction
MIT License
278 stars 30 forks source link

PR for issue 27 - adding multiple wildcard support #36

Closed champagnetony closed 2 years ago

champagnetony commented 3 years ago

Coverage is not 100% but I've had some trouble figuring out how to get the extra branches covered in the multi-wildcard state. Any suggestions would be appreciated. Based on my limited understanding of the code base, this approach does work, but I could see that there could be better ways to tackle this problem in a more programmatically clean design.

champagnetony commented 3 years ago

Have you run the benchmarks before/after your change?

fast-redact-bench-27.txt fast-redact-bench-master.txt

davidmarkclements commented 3 years ago

@champagnetony if you run npm run cov-ui you'll get a visual coverage report, this should make it easier to figure out what tests still need to be written

champagnetony commented 3 years ago

@champagnetony if you run npm run cov-ui you'll get a visual coverage report, this should make it easier to figure out what tests still need to be written

Thank you - There are only 3 lines that need the various paths covered, but I just don't know how to exercise those paths. Any suggestions would be welcome.

Screen Shot 2021-05-03 at 15 39 18
andrhamm commented 3 years ago

@champagnetony Do you expect you'll have any time to complete the test coverage? Multiple wildcard support would be great to have! Thanks

jsumners commented 3 years ago

Anyone can pick up the work and retain authorship of prior work -- https://gist.github.com/jsumners/461ef7a64545108635cc437fde112721

champagnetony commented 3 years ago

@champagnetony Do you expect you'll have any time to complete the test coverage? Multiple wildcard support would be great to have! Thanks

I've been trying to get back to it but work has gotten busier and then I've been out of town a lot. If you'd like to finish the tests, please go ahead.

lrecknagel commented 2 years ago

I´ve tried my best to finish this PR as advised, see #40 Happy to improve if still things remain.

champagnetony commented 2 years ago

I´ve tried my best to finish this PR as advised, see #40 Happy to improve if still things remain.

Thank you for picking this up!