awakesecurity/proto3-wire

Fix parsedField comment or code

ndmitchell opened this issue · 3 comments

Looking at

-- | To comply with the protobuf spec, if there are multiple fields with the same
-- field number, this will always return the last one.
parsedField :: RawField -> Maybe RawPrimitive
parsedField xs = case xs of
[] -> Nothing
(x:_) -> Just x

It says "To comply with the protobuf spec, if there are multiple fields with the same field number, this will always return the last one" and then returns the first one. That seems to be an optimisation from @gbaz. Was that intentional? If so, perhaps update the comment? If not, perhaps update the code? Our optimised version used lastMay there.

gbaz commented

If I recall, we keep the list in reversed order. As we approach Easter, it is appropriate that in this code, the last shall be first.

If that''s the case, we should document it on the RawField type, and update the comment to parsedField. Might be worth adding a test though, since changing it to be the last doesn't seem to break anything for me.

gbaz commented

It was also suggested we move to a newtype, which would be even clearer in this regard...