search

mennanov / fmutils

Golang protobuf FieldMask missing utils
MIT License
97 stars 10 forks source link

Update fmutils.go #1

Closed SachsA closed 3 years ago

SachsA commented 3 years ago

This condition was missing when trying to filter on nested structs it was panicing.

codecov[bot] commented 3 years ago

Codecov Report

Merging #1 (cca613b) into main (9e8bf86) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main        #1   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           51        51           
=========================================
  Hits            51        51
Impacted Files Coverage Δ
fmutils.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9e8bf86...cca613b. Read the comment docs.

mennanov commented 3 years ago

Please add a test that covers this case.

SachsA commented 3 years ago

Yes of course. Sorry I just realized that I didn't commit them... Forgive me. I will do it tomorrow.

SachsA commented 3 years ago

@mennanov There you go, I added some tests. Hope this is ok for you. Feel free to reach me if you need.

mennanov commented 3 years ago

Your pull request actually revealed that the code does not currently support an important use case: masks in repeated non-scalar fields. I created a pull request to address this issue: https://github.com/mennanov/fmutils/pull/2 It should also fix the problem this pull request addresses. Please, take a look at that pull request and let me know if it works for you. If everything is OK i'll merge my pull request in favor of this one.

Thanks for contributing!