Support for empty symbols, e.g. '' by raganhan · Pull Request #108 · amazon-ion/ion-java

raganhan · 2017-07-21T21:01:33Z

Issue: #42

toddjonker · 2017-07-21T22:22:30Z

I definitely want to review this, hopefully this weekend. Stoked this is getting fixed!

tgregg

A few fixes requested in the tests.
Suggestion to factor the style-related changes (import order, variable renames) into a separate PR to make this one smaller and easier to review.
Other than that, the related fixes look good.

tgregg · 2017-07-24T20:08:23Z


 package software.amazon.ion;

+import software.amazon.ion.system.IonTextWriterBuilder;


It's probably best to configure your IDE so that it doesn't change the import order in this repo. It's bloating the size of the diff and has the potential to be reversed in a subsequent PR.

I separated the code style changes and fiddled a bit with my IDE settings to avoid this in the future.

Do we have a style guide I can use as reference?

For posterity: the Eclipse preferences stored in .settings. We can store preferences for IntelliJ too if needed.

tgregg · 2017-07-24T20:13:55Z

+            break;
        case 'f':
-            if (valuelen == 5 //      'f'
+            if (valueLength == 5 //      'f'


Before the variable was renamed, the 'f' was in the same column as the 'alse' that follow. Please either fix that or leave the rename for a separate style-related PR.

Separated the code style changes

tgregg · 2017-07-24T20:16:30Z

-        if (text == null || text.length() == 0)
+        if (text == null)
        {
            throw new EmptySymbolException();


This should be NPE or IllegalArgumentException. Then we can eliminate EmptySymbolException altogether.

I agree this would be better as NPE or IAE. However I was avoiding removing EmptySymbolException whenever possible as it breaks binary compatibility for some interfaces, since EmptySymbolException is a checked exception.

I'm pro removing it as a whole if we consider are OK with this kind of compatibility break

What did you determine the impact to be? Will the new code compile for users who explicitly check for EmptySymbolException?

tgregg · 2017-07-24T20:19:48Z

-            fail("expected exception");
-        }
-        catch (EmptySymbolException e) { }
+        iw.writeSymbol("a");


This isn't writing an empty symbol, as the name suggests.

tgregg · 2017-07-24T20:21:01Z

+        throws Exception
+    {
+        iw = makeWriter();
+        iw.writeSymbol("");


This isn't setting a field name, it's writing a symbol value.

tgregg · 2017-07-25T18:43:45Z

-        IonReaderBinarySpan pos = position;
-
-        if (pos == null)
+        if (position == null)


I'd suggest leaving style fixes for an independent PR.

tgregg · 2017-07-25T18:52:31Z

-    private final String ann = "ann";
-    private final String ben = "ben";
+    private static final String ANN = "ANN";
+    private static final String BEN = "BEN";


Not related; suggest separate PR.

tgregg · 2017-07-25T18:54:21Z

        int sid = value.symbolValue().getSid();

-        try {
-            value.setValue("");


Is there a positive test for this now?

tgregg · 2017-07-25T19:12:58Z

    @Test
    public void testSymbolWithEscapedNewline()
    {
-        badValue("'\\\n'");


This used to fail because \\\n is an escaped newline, which expands to nothing within quoted text, leaving this as an empty symbol value. We should be testing that that case is now valid (and is treated as an empty symbol). This might actually be a good addition to ion-tests if it's not already there.

"'\\n'" is also an interesting test; it's just a different test.

changed it by mistake, added it back again as a valid case. Will take a look if this is included in ion-tests and add it in a separate PR if necessary

issue: #42

@deprecated

I was mistaken this is a unchecked exception so removal is fine. Kept the class as @deprecated to avoid breaking clients

raganhan · 2017-07-26T17:35:30Z

It works fine because EmptySymbolException is actually a RuntimeException. I thought it was a checked Exception because it was present in some method signatures, but that's not really the case. For checked exceptions trying to catch something that's not thrown is a compilation error Example: https://gist.github.com/raganhan/e89c10169e62b4546c812134f3843d80

…

On Tue, Jul 25, 2017 at 5:45 PM Tyler Gregg ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/software/amazon/ion/impl/PrivateUtils.java <#108 (comment)>: > @@ -228,10 +162,11 @@ public static SymbolToken newSymbolToken(SymbolTable symtab, String text) { // TODO amazon-ion/ion-java#21 symtab should not be null - if (text == null || text.length() == 0) + if (text == null) { throw new EmptySymbolException(); What did you determine the impact to be? Will the new code compile for users who explicitly check for EmptySymbolException? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA_NfKAl397Ac80OAUUcYmEyyDAQ_teks5sRowWgaJpZM4Of437> .

Support for empty symbols, e.g. '' Empty symbols are valid according to the spec so they need to be allowed. Removed usage of EmptySymbolException as it was only used in two cases: * Empty symbols which are now supported * Java null symbols which were converted to NPEs. EmptySymbolException was kept as deprecated to avoid breaking any client that depends on it issue: #42

raganhan requested review from a team and tgregg July 21, 2017 21:01

toddjonker self-requested a review July 21, 2017 22:22

tgregg requested changes Jul 25, 2017

View reviewed changes

Support for empty symbols, e.g. ''

e5fa879

issue: #42

raganhan force-pushed the support-empty-symbol-42 branch from 0687f34 to e5fa879 Compare July 25, 2017 21:28

raganhan added 2 commits July 25, 2017 14:44

fixing testSetEmptyFieldName

3b4b9ee

Removing EmptySymbolException in favor of NPE

6ea9d89

I was mistaken this is a unchecked exception so removal is fine. Kept the class as @deprecated to avoid breaking clients

tgregg approved these changes Jul 26, 2017

View reviewed changes

raganhan merged commit 3026661 into master Jul 26, 2017

raganhan deleted the support-empty-symbol-42 branch July 26, 2017 17:39


		package software.amazon.ion;

		import software.amazon.ion.system.IonTextWriterBuilder;

Conversation

raganhan commented Jul 21, 2017

Uh oh!

toddjonker commented Jul 21, 2017

Uh oh!

tgregg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raganhan commented Jul 26, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants