Support proto maps stringification #383

illia-shepilov · 2017-03-10T10:02:20Z

Summary of changes:

Created the MapStringifier class.
Created the tests for the MapStringifier class.
Created the Types utility class.

The usage of the MapStringifier:

// Stringifier creation.
final Stringifier<Map<String, Long>> mapStringifier = Stringifiers.mapStringifier(String.class, Long.class);

// The registration of the stringifier.
final Type type = new TypeToken<Map<String, Long>>(){}.getType();
StringifierRegistry.getInstance().register(stringifier, type);

// Obtain already registered `MapStringifier`.
final Type type = Types.mapTypeOf(String.class, Long.class);
final Stringifier<Map<String, Long>> mapStringifier = StringifierRegistry.getInstance().getStringifier(type);

// Convert to string.
final Map<String, Long> mapToConvert = newHashMap();
mapToConvert.put("first", 1);
mapToConvert.put("second", 2);
 
// The result is: \"first\":\"1\",\"second\":\"2\".
final String convertedString = mapStringifier.toString(mapToConvert);

// Convert from string.
final String stringToConvert = ...
final Map<String, Long> convertedMap = mapStringifier.fromString(stringToConvert);

…hub.com/SpineEventEngine/core-java into support-proto-maps-stringification

codecov · 2017-03-10T10:02:22Z

Codecov Report

Merging #383 into master will increase coverage by 0.05%.
The diff coverage is 95.65%.

@@             Coverage Diff             @@
##             master    #383      +/-   ##
===========================================
+ Coverage     91.84%   91.9%   +0.05%     
- Complexity     2395    2428      +33     
===========================================
  Files           240     243       +3     
  Lines          7750    7842      +92     
  Branches        558     562       +4     
===========================================
+ Hits           7118    7207      +89     
- Misses          486     490       +4     
+ Partials        146     145       -1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94393b7...d1adbac. Read the comment docs.

…hub.com/SpineEventEngine/core-java into support-proto-maps-stringification.

…ass.

…hub.com/SpineEventEngine/core-java into support-proto-maps-stringification.

alexander-yevsyukov

Please see my comments, add coverage, and escaping of delimiters in the input strings.

alexander-yevsyukov · 2017-03-10T16:39:36Z

client/src/main/java/org/spine3/base/Stringifiers.java

+        }
+
+        @SuppressWarnings("unchecked") // It is OK because class is verified.
+        private static <I> I getConvertedElement(Class<I> elementClass, String elementToConvert) {


You don't get here, but convert.

alexander-yevsyukov · 2017-03-10T16:40:22Z

client/src/main/java/org/spine3/base/Stringifiers.java

+        }
+    }
+
+    private static boolean isStringClass(Class<?> aClass) {


Why not isString?

…into support-proto-maps-stringification

alexander-yevsyukov

Please address my minor comments, and it would be LGTM.

alexander-yevsyukov · 2017-03-23T16:02:04Z

client/src/main/java/org/spine3/base/MapStringifier.java

+        if (keyValue.length != 2) {
+            final String exMessage =
+                    "Illegal key-value format. The value should " +
+                    "be separated with a single `:` character.";


Please have constant for the separator character and use it in the message. Consider avoiding back-ticks. You can name the character (colon) and then quote it.

alexander-yevsyukov · 2017-03-23T16:02:25Z

client/src/main/java/org/spine3/base/MapStringifier.java

+        }
+    }
+
+    @SuppressWarnings("unchecked") // It is OK because class is verified.


What do you mean by “verified”?

illia-shepilov · 2017-03-23T16:24:09Z

@alexander-yevsyukov PTAL.

alexander-yevsyukov

As discussed vocally, please store key value in quotes with colon as the separator (which would be similar to Json).

alexander-yevsyukov

Please see one more question.

alexander-yevsyukov · 2017-03-24T09:33:06Z

client/src/main/java/org/spine3/base/Stringifiers.java

+        @SuppressWarnings("unchecked") // It is OK because class is verified.
+        private static <I> I convert(Class<I> elementClass, String elementToConvert) {
+
+            if (isInteger(elementClass)) {


Why don't we add stringifiers for well-known types instead?

illia-shepilov · 2017-03-24T13:08:28Z

@alexander-yevsyukov PTAL.

alexander-yevsyukov

Please see my comments and questions.

alexander-yevsyukov · 2017-03-24T17:34:42Z

client/src/main/java/org/spine3/base/MapStringifier.java

+                    new AbstractMap.SimpleEntry<>(convertedKey, convertedValue);
+            return convertedBucket;
+        } catch (Throwable e) {
+            throw newIllegalArgumentException("The exception is occurred during the conversion", e);


"The exception is occurred during the conversion"

Without ”is” it would read a bit better. Please also test this case.

It is tested with the throw_exception_when_passed_parameter_does_not_match_expected_format test.
But CodeCove extension shows it as uncovered.

alexander-yevsyukov · 2017-03-24T17:35:19Z

client/src/main/java/org/spine3/base/ConversionException.java

+    private static final long serialVersionUID = 1L;
+
+    public ConversionException(String message, Throwable cause) {
+        super(message, cause);


Please add test coverage.

That exception is not used in that PR, but it is used in the next PR according to stringifiers.

alexander-yevsyukov · 2017-03-24T17:38:13Z

client/src/main/java/org/spine3/base/MapStringifier.java

+        return unquotedValue;
+    }
+
+    private static boolean isQuotedKeyValue(String[] keyValue) {


Why do we pass a string array here? Why not two strings?

alexander-yevsyukov · 2017-03-24T17:38:28Z

client/src/main/java/org/spine3/base/MapStringifier.java

+        final int keyLength = key.length();
+        final int valueLength = value.length();
+
+        if (keyLength < 2 || valueLength < 2) {


Please test all branches.

alexander-yevsyukov · 2017-03-24T17:40:49Z

client/src/main/java/org/spine3/base/MapStringifier.java

+            return false;
+        }
+
+        final boolean result = isQuote(key.charAt(1)) && isQuote(key.charAt(keyLength - 1)) &&


I would add a utility method called isQuotedString(). Please check for such a thing already available.

alexander-yevsyukov · 2017-03-24T17:42:08Z

client/src/main/java/org/spine3/base/MapStringifier.java

+        final String escapedChar = "\\" + charToEscape;
+        final Escaper result = Escapers.builder()
+                                       .addEscape('\"', "\\\"")
+                                       .addEscape(':', "\\:")


Why do we still need to escape the colon character?

alexander-yevsyukov · 2017-03-24T17:43:31Z

client/src/main/java/org/spine3/base/Stringifiers.java

+    static class LongStringifier extends Stringifier<Long>{
+        @Override
+        protected String toString(Long obj) {
+            return Longs.stringConverter().reverse().convert(obj);


Please format the code here and other similar places.

alexander-yevsyukov · 2017-03-24T17:45:06Z

client/src/main/java/org/spine3/base/Types.java

+
+        final Type type =  new TypeToken<Map<K, V>>() {}.where(new TypeParameter<K>() {}, keyClass)
+                                                        .where(new TypeParameter<V>() {}, valueClass)
+                                                        .getType();


Круто! 👍

illia-shepilov · 2017-03-25T22:27:33Z

@alexander-yevsyukov PTAL.

alexander-yevsyukov

Please address code formatting comments before merging. The rest LGTM.

alexander-yevsyukov · 2017-03-27T10:31:33Z

client/src/test/java/org/spine3/base/MapStringifierShould.java

@@ -116,8 +116,36 @@ public void convert_string_which_contains_delimiter_in_content_to_map(){
    }

    @Test(expected = IllegalArgumentException.class)
-    public void throw_exception_when_key_value_are_incorrect(){
+    public void throw_exception_when_value_has_not_prior_quote(){


Please allow one space char before { here and in the methods below.

alexander-yevsyukov · 2017-03-27T10:32:16Z

client/src/main/java/org/spine3/base/MapStringifier.java

        return result;
    }

-    private static boolean isQuote(char character) {
+    private static boolean isQuote(char character){


Please add one space char before {.

illia-shepilov added 3 commits March 10, 2017 11:22

Create MapStringifier.

c2fbed3

Merge branch 'stringifiers-and-identifiers-separation' of https://git…

0f60da5

…hub.com/SpineEventEngine/core-java into support-proto-maps-stringification

Finish implementation of the MapStringifier class.

64365d6

illia-shepilov added 7 commits March 10, 2017 12:41

Fix the implementation.

45a7319

Create tests for the MapStringifier class.

8c2fb03

Merge branch 'stringifiers-and-identifiers-separation' of https://git…

1bf583d

…hub.com/SpineEventEngine/core-java into support-proto-maps-stringification.

Move generating and throwing exception message to the Exceptions cl…

c07f008

…ass.

Fix the documentation.

d6be339

Fix the implementation.

95f1f5e

Add check for the method parameter.

6a0aff0

illia-shepilov mentioned this pull request Mar 10, 2017

Stringifiers and Identifiers separation. #382

Merged

illia-shepilov added 2 commits March 10, 2017 15:05

Add the processing of the exceptions.

b85473d

Add tests for the MapStringifier class.

3aaf0e1

illia-shepilov self-assigned this Mar 10, 2017

illia-shepilov added 8 commits March 10, 2017 15:12

Add test for the Exceptions class.

85a4302

Merge branch 'stringifiers-and-identifiers-separation' of https://git…

0f4030d

…hub.com/SpineEventEngine/core-java into support-proto-maps-stringification.

Fix MapStringifier according to the new functionality.

80d2c33

Add test for the MapStringifier class.

140afdd

Fix exception message.

4b7fca8

Add test for the MapStringifier class.

4aedb1a

Extract repeated exception message to constant.

88e1564

Fix the test implementation.

3aee1de

alexander-yevsyukov requested changes Mar 10, 2017

View reviewed changes

illia-shepilov added 4 commits March 10, 2017 18:51

Merge branch 'master' of https://github.com/SpineEventEngine/core-java …

e828210

…into support-proto-maps-stringification

Rename method from getConvertedElement to convert.

35b698c

Rename methods.

bfec14f

Fix test.

19a9767

illia-shepilov changed the base branch from stringifiers-and-identifiers-separation to master March 10, 2017 17:08

Fix tests.

d224fd4

alexander-yevsyukov requested changes Mar 23, 2017

View reviewed changes

illia-shepilov added 4 commits March 23, 2017 18:13

Fix the issues.

1f02807

Reformat code.

5b529e7

Move the character to the constant.

4c15917

Rename the constant.

92ed231

alexander-yevsyukov requested changes Mar 23, 2017

View reviewed changes

alexander-yevsyukov requested changes Mar 24, 2017

View reviewed changes

illia-shepilov added 6 commits March 24, 2017 14:42

Rewrite implementation for the MapStringifier.

aa125d2

Create the stringifiers for the long and int values.

e8946ca

Remove unused methods.

7064fde

Fix the documentation.

56d160e

Add test for the MapStringifier class.

2e9f9e7

Add tests for the long and int stringifiers.

1b696dc

alexander-yevsyukov requested changes Mar 24, 2017

View reviewed changes

illia-shepilov added 8 commits March 25, 2017 23:01

Remove the useless escaping of the key-value.

53917b7

Reformat code.

f412f88

Create the utility method.

cbdce85

Fix the grammar issue.

6a32dd4

Fix the implementation of the utility method.

e7cf791

Add test for the MapStringifier class.

795e215

Add tests for the MapStringifier class.

ca6eba2

Add test for the MapStringifier class.

9f0dc5b

alexander-yevsyukov approved these changes Mar 27, 2017

View reviewed changes

illia-shepilov and others added 2 commits March 27, 2017 13:47

Reformat the code.

fbc05e0

Merge branch 'master' into support-proto-maps-stringification

d1adbac

illia-shepilov merged commit c5e41b6 into master Mar 27, 2017

illia-shepilov deleted the support-proto-maps-stringification branch March 27, 2017 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support proto maps stringification #383

Support proto maps stringification #383

illia-shepilov commented Mar 10, 2017 •

edited

Loading

codecov bot commented Mar 10, 2017 •

edited

Loading

alexander-yevsyukov left a comment

alexander-yevsyukov Mar 10, 2017

alexander-yevsyukov Mar 10, 2017

alexander-yevsyukov left a comment

alexander-yevsyukov Mar 23, 2017

alexander-yevsyukov Mar 23, 2017

illia-shepilov commented Mar 23, 2017

alexander-yevsyukov left a comment

alexander-yevsyukov left a comment •

edited

Loading

alexander-yevsyukov Mar 24, 2017

illia-shepilov commented Mar 24, 2017

alexander-yevsyukov left a comment

alexander-yevsyukov Mar 24, 2017

illia-shepilov Mar 25, 2017 •

edited

Loading

alexander-yevsyukov Mar 24, 2017

illia-shepilov Mar 25, 2017 •

edited

Loading

alexander-yevsyukov Mar 24, 2017

alexander-yevsyukov Mar 24, 2017

alexander-yevsyukov Mar 24, 2017

alexander-yevsyukov Mar 24, 2017

alexander-yevsyukov Mar 24, 2017

alexander-yevsyukov Mar 24, 2017

illia-shepilov commented Mar 25, 2017

alexander-yevsyukov left a comment

alexander-yevsyukov Mar 27, 2017

alexander-yevsyukov Mar 27, 2017

Support proto maps stringification #383

Support proto maps stringification #383

Conversation

illia-shepilov commented Mar 10, 2017 • edited Loading

codecov bot commented Mar 10, 2017 • edited Loading

Codecov Report

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-shepilov commented Mar 23, 2017

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

alexander-yevsyukov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-shepilov commented Mar 24, 2017

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-shepilov Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-shepilov Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-shepilov commented Mar 25, 2017

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-shepilov commented Mar 10, 2017 •

edited

Loading

codecov bot commented Mar 10, 2017 •

edited

Loading

alexander-yevsyukov left a comment •

edited

Loading

illia-shepilov Mar 25, 2017 •

edited

Loading

illia-shepilov Mar 25, 2017 •

edited

Loading