Override features defined in enclosed scopes. #479
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…troduced by Protobuf Editions. Also fixed a bug with DELIMITED encoding, i.e., the encoding for proto2 groups, that was detected by the rewritten tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+145
to
+156
| [ const (testCase "NONE (skipped)" (pure ())) $ | ||
| -- We actually cannot disable validation of UTF-8 in strings. | ||
| -- This could be done by passing down the verification mode | ||
| -- to the code generator and using decodeUtf8Lenient instead | ||
| -- when verification is disabled. This may not be worth it; | ||
| -- the content with a string containing invalid UTF-8 is undefined, | ||
| -- and changing it to a bytes field would be a straightforward fix. | ||
| -- | ||
| -- If this was done, we would stop ignoring this test case. | ||
| deserializeFrom "NONE" | ||
| (Just $ defFeatures & utf8ValidationNone .~ "depends on decoding") | ||
| (tagged 8 $ Lengthy invalidUtf8) |
Check warning
Code scanning / HLint
Evaluate Warning test
Comment on lines
+169
to
+180
| , const (testCase "LENGTH_PREFIXED (skipped)" (pure ())) $ | ||
| -- proto-lens sets DELIMITED encoding by message type, not field. | ||
| -- Full compatability with Protobuf Editions would require it to be | ||
| -- by field. However, given the usual use case for DELIMITED encoding, | ||
| -- where it is to support legacy proto2 groups in which message types | ||
| -- and fields are tied toggether, this may not be worth fixing. | ||
| -- | ||
| -- If this was done, we would stop ignoring this test case. | ||
| serializeTo "LENGTH_PREFIXED" | ||
| (defFeatures & messageEncodingLengthPrefixed . foo .~ 21) | ||
| (braced "message_encoding_length_prefixed" $ keyedInt "foo" 21) | ||
| (tagged 11 $ Lengthy $ tagged 1 $ VarInt 21) |
Check warning
Code scanning / HLint
Evaluate Warning test
|
@agrue, this last change completes support for Protobuf Editions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This completes support of Protobuf Editions to the same level that proto-lens supports proto2 and proto3 except for the following, which I do not think are critical bugs:
features.utf8_validation = NONEdoes not disable UTF-8 validation.If more than one field sharing the same message type are in a single message type, and if one of the fields has
features.message_encoding = DELIMITED, then all of these fields will useDELIMITEDencoding.Unevaluated tests are intentionally included despite warnings by HLint so that they are still built and can be easily enabled later if the above are fixed.
Resolves #468.