Conversation
| test: { | ||
| name: "bytes support for string with invalid utf-8 encoding" | ||
| expr: '"%s".format([b"\xF0abc\x8C\xF0xyz"])' | ||
| expr: '"%s".format([b"\\xF0abc\\x8C\\xF0xyz"])' |
There was a problem hiding this comment.
Drive-by change. I realized in Python that loading this file with actual invalid utf-8 bytes will prevent it from being loaded. So, just escaping the \x so that it loads as a normal string. Updated the PR to the cel-spec also.
| "object not allowed", | ||
| // found no matching overload for 'format' applied to 'string.(list(map(int, dyn)))' | ||
| "object inside map"); | ||
| private static List<String> SKIPPED_TESTS = Arrays.asList(); |
There was a problem hiding this comment.
Don't love that this is here an unused. We could remove it, but I like having it here in case we need it. If there's a better way, I'm all ears.
There was a problem hiding this comment.
nit: Could use Collections.emptyList(), but might be good just to remove?
| StringBuilder builder = new StringBuilder(); | ||
| builder.append('{'); | ||
|
|
||
| List<String> entries = new ArrayList<String>(); |
There was a problem hiding this comment.
| List<String> entries = new ArrayList<String>(); | |
| List<String> entries = new ArrayList<>(); |
There was a problem hiding this comment.
👍 fixed instances in FormatTest also.
| // The formatted string should be sorted by keys | ||
| Collections.sort(entries); | ||
|
|
||
| builder.append(String.join(", ", entries)).append('}'); |
There was a problem hiding this comment.
nit: Using a builder isn't really that efficient this way since we're still building all the individual strings and joining them. I don't think this is going to be very performance sensitive code but might be good to optimize it at some point.
| builder.append('}'); | ||
|
|
||
| // The formatted string should be sorted by keys | ||
| Collections.sort(entries); |
There was a problem hiding this comment.
Sorting the keys after we've concatenated the : <val> won't always give what you expect:
public static void main(String[] args) {
List<String> keys = Arrays.asList("a9", "a");
Collections.sort(keys);
System.out.println(keys);
List<String> vals = Arrays.<String>asList("a9: val", "a: val");
Collections.sort(vals);
System.out.println(vals);
}This prints different values for sorting by keys vs. sorting by key values:
[a, a9]
[a9: val, a: val]
There was a problem hiding this comment.
Great catch, thank you.
| "object not allowed", | ||
| // found no matching overload for 'format' applied to 'string.(list(map(int, dyn)))' | ||
| "object inside map"); | ||
| private static List<String> SKIPPED_TESTS = Arrays.asList(); |
There was a problem hiding this comment.
nit: Could use Collections.emptyList(), but might be good just to remove?
2 participants
After some investigation, it seems the reason some of the cel conformance tests weren't running was because the overload declaration for
formatinside protovalidate-java was too narrow and was only allowing primitive types inside lists.This simplifies the declaration so that any
DynTypeis accepted, which allows maps as well as custom Protobuf types to be passed inside a list to format.