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

Fix parsedField comment or code #51

Open
ndmitchell opened this issue Apr 16, 2019 · 3 comments
Open

Fix parsedField comment or code #51

ndmitchell opened this issue Apr 16, 2019 · 3 comments

Comments

@ndmitchell
Copy link

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
Copy link
Collaborator

gbaz commented Apr 16, 2019

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.

@ndmitchell
Copy link
Author

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
Copy link
Collaborator

gbaz commented Apr 16, 2019

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

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

No branches or pull requests

2 participants