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

Add methodOptions to HasMethodImpl to provide custom method options. #331

Merged
merged 4 commits into from
Aug 30, 2019

Conversation

judah
Copy link
Collaborator

@judah judah commented Aug 30, 2019

We provide the whole proto message, rather than pulling off individual fields. The set of fields may change for different builds of proto-lens (or based on extensions, though we can't see those directly due to #27).

Internally, we save the proto message in the source file as an encoded string, and decode it into a Haskell type lazily.

judah added 3 commits August 28, 2019 16:07
Next up: encode the MethodOptions as a string and decode it lazily
@judah judah requested a review from jinwoo August 30, 2019 19:06
Copy link
Member

@jinwoo jinwoo left a comment

Choose a reason for hiding this comment

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

LGTM but there seems to be a redundant pattern matching error with some GHC versions.

@judah
Copy link
Collaborator Author

judah commented Aug 30, 2019

Fixed; PTAL.

It turns out the test (which relied on GHC's overlapping pattern matching) wasn't catching errors correctly:

-- This module will fail to compile due to @-fwarn-overlapping-patterns@ and

It was worse on newer GHC, but even on older ones it missed a misuse of "bidiStreaming" instead of `"biDiStreaming":

I fixed it by just calling evaluate on the values to force GHC to resolve the constraints.

@judah judah merged commit 9fa8924 into master Aug 30, 2019
@judah judah deleted the method-options branch August 30, 2019 22:48
judah added a commit that referenced this pull request Oct 15, 2019
We're not using it currently.  Also, the existing approach
doesn't work well with our internal Blaze rules.  If we still
want this long-term, we can revisit it after fixing #334.
judah added a commit that referenced this pull request Oct 15, 2019
We're not using it currently.  Also, the existing approach
doesn't work well with our internal Blaze rules.  If we still
want this long-term, we can revisit it after fixing #334.
judah added a commit that referenced this pull request Oct 15, 2019
We're not using it currently.  Also, the existing approach
doesn't work well with our internal Blaze rules.  If we still
want this long-term, we can revisit it after fixing #334.
judah added a commit that referenced this pull request Oct 16, 2019
We're not using it currently.  Also, the existing approach
doesn't work well with our internal Blaze rules.  If we still
want this long-term, we can revisit it after fixing #334.
avdv pushed a commit to avdv/proto-lens that referenced this pull request Aug 9, 2023
…ns. (google#331)

We provide the whole proto message, rather than pulling off individual fields. The set of fields may change for different builds of proto-lens (or based on extensions, though we can't see those directly due to google#27).

Internally, we save the proto message in the source file as an encoded string, and decode it into a Haskell type lazily.
avdv pushed a commit to avdv/proto-lens that referenced this pull request Aug 9, 2023
We're not using it currently.  Also, the existing approach
doesn't work well with our internal Blaze rules.  If we still
want this long-term, we can revisit it after fixing google#334.
ylecornec pushed a commit to ylecornec/proto-lens that referenced this pull request Feb 19, 2024
…ns. (google#331)

We provide the whole proto message, rather than pulling off individual fields. The set of fields may change for different builds of proto-lens (or based on extensions, though we can't see those directly due to google#27).

Internally, we save the proto message in the source file as an encoded string, and decode it into a Haskell type lazily.
ylecornec pushed a commit to ylecornec/proto-lens that referenced this pull request Feb 19, 2024
We're not using it currently.  Also, the existing approach
doesn't work well with our internal Blaze rules.  If we still
want this long-term, we can revisit it after fixing google#334.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants