Conversation
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
============================================
+ Coverage 80.02% 80.26% +0.23%
- Complexity 1681 1711 +30
============================================
Files 156 158 +2
Lines 5597 5684 +87
Branches 731 748 +17
============================================
+ Hits 4479 4562 +83
- Misses 839 840 +1
- Partials 279 282 +3
Continue to review full report at Codecov.
|
alarribeau
left a comment
alarribeau
left a comment
There was a problem hiding this comment.
It's neat :)
I have just made an optional comment.
| FieldType.BOOLEAN, | ||
| FieldType.INT8, FieldType.INT16, FieldType.INT32, FieldType.INT64, | ||
| FieldType.FLOAT, FieldType.DOUBLE, | ||
| FieldType.DATE, FieldType.DATE_TIME |
There was a problem hiding this comment.
I think it could be cleaner to move this logic into the FieldType enum by adding a new field :
private final boolean serializeableIntoString; // or scalarValue?
There was a problem hiding this comment.
I actually prefer the current solution: the list of types usable as Map keys is somehow arbitrary, and might evolve over time. It is not intrinsic to the FieldTypes, on the contrary of the other existing fields.
| * object (see {@link com.google.api.server.spi.config.model.SchemaRepository#MAP_SCHEMA}). | ||
| * | ||
| * To enable one of these enum flags, you can either: | ||
| * - Set system property "endpoints.mapSchema." + systemPropertyName to any value except a falsy one |
There was a problem hiding this comment.
systemPropertyName() ?
MapSchemaFlag.systemPropertyName() ?
(It wasn't clear for me at first read what was this systemPropertyName since I read the description before the code)
There was a problem hiding this comment.
The is indeed a confusion here, as we have constructor param and field with same name but different values. Renamed constructor param to systemPropertySuffix, and improved documentation.
| FieldType keyFieldType = FieldType.fromType(Types.getTypeParameter(type, 0)); | ||
| boolean supportedKeyType = SUPPORTED_MAP_KEY_TYPES.contains(keyFieldType); | ||
| if (!supportedKeyType) { | ||
| String message = "Map field type '" + type + "' has a key type not serializable to String"; |
There was a problem hiding this comment.
It would make this slightly more compact too :
boolean supportedKeyType = SUPPORTED_MAP_KEY_TYPES.contains(keyFieldType);
if (!supportedKeyType) {
=>
if (!keyFieldType.isSerializableToString()) {
clementdenis
force-pushed
the
map_support_in_generators
branch
from
14c1d58 to
ee0b460
Compare
- Two unit tests disabled (to reenable) - Missing flag to enable new map handling - Changed description handling in SwaggerGenerator to follow spec
- Enable new behaviour with a property / env flag - Still use JsonMap for unsupported keys (not String) or value types (arrays) - Add a flag to warn about unsupported Map type parameters - Reenable disabled tests (now passing) - Test old and new behaviour in unit tests
- Support additional map key types (confirmed to work at runtime) - Add new flag to enable support for array-like value types - ALlow to use warn flag without actually generating new schema for Maps - Add Javadoc on flags for maps - Additional test cases for Map types
- Improve Map schema-related flags with dedicated MapSchemaFlag class - Default to Map schema using "additionalProperties", with flag for legacy mode - Fail by default when encountering non-String Map keys, with flag to ignore - Improve unit tests
- Handle subclasses of Map with concrete key and value (exclude others) - Exclude raw Map type - Better support nested Maps
clementdenis
force-pushed
the
map_support_in_generators
branch
from
ee0b460 to
1e573c8
Compare
|
Fixed Swagger tests after merging cloudendpoints#155. |
4 participants
No description provided.