Add BiMap to the special support for collections and maps. #419

thespags · 2020-02-24T07:00:03Z

Follows the behavoir of Maps under https://github.com/inferred/FreeBuilder#collections-and-maps.

Addresses #418

thespags · 2020-02-24T07:00:38Z

Hi @alicederyn here's a rough draft of adding BiMaps. I do have some questions inlined for you. Mostly I went the lazy approach of using the existing implementation for Map and moving it over to BiMap.

src/test/java/org/inferred/freebuilder/processor/property/BiMapPropertyTest.java

thespags · 2020-02-24T07:03:17Z

src/main/java/org/inferred/freebuilder/processor/property/BiMapProperty.java

+    addPut(code);
+    addPutAll(code);
+    addRemoveKey(code);
+    addRemoveValue(code);


This may have been misguided but since BiMap's are 1-1, you may want to make modifications based on the value and not the key.

Seems reasonable. You could do .mutateX(m -> m.inverse().remove(value)) so it really comes down to convenience.

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java

alicederyn

Answers and some points I noted in a quick skim. Please make sure you have the full API tested — I noted some examples but I wasn't exhaustive.

src/main/java/org/inferred/freebuilder/processor/property/BiMapProperty.java

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java

alicederyn · 2020-02-24T13:35:01Z

src/main/java/org/inferred/freebuilder/processor/property/BiMapProperty.java

+    addPut(code);
+    addPutAll(code);
+    addRemoveKey(code);
+    addRemoveValue(code);


Seems reasonable. You could do .mutateX(m -> m.inverse().remove(value)) so it really comes down to convenience.

alicederyn · 2020-02-24T13:36:58Z

src/main/java/org/inferred/freebuilder/processor/property/BiMapProperty.java

+        .addLine(
+            "public %s %s(%s key) {",
+            datatype.getBuilder(),
+            "removeKey" + property.getCapitalizedName(),


"removeKeyFrom" + ... seems more readable — also, should live in BuilderMethods.

The "from" reads much better, but doesn't the method currently live in BuilderMethods?

You'll need to add a new static method, removeKeyFromMethod, to BuilderMethods.java, so that you're not writing out the formula "removeKeyFrom" + x everywhere.

src/main/java/org/inferred/freebuilder/processor/property/BiMapProperty.java

src/test/java/org/inferred/freebuilder/processor/property/BiMapPropertyTest.java

thespags · 2020-03-01T17:24:18Z

I'd suggest passing around forcePut instead of put, and doing the IllegalArgumentException-throwing check of put explicitly in the put method. You'll also need tests.

If I understand it correctly, that means defining a forcePut method on the builder. As the passed around put is simply the implementation defined (overridden or not) for the builder.
And adding

      checkArgument(!containsValue(value), "value already present: %s", value);

to the put in the checked bimap.

Is that correct?

alicederyn · 2020-03-05T13:27:52Z

Yes, that seems correct: generate a forcePutX (name creation logic should live in BuilderMethods.forcePutMethod) method, and use it as the canonical put method.

Note that this also means that the generated putX needs to be rewritten to delegate to forcePutX, as the point of this circumlocution is to support https://github.com/inferred/FreeBuilder#defaults-and-constraints — the user overrides a single method, adds custom constraint checking, and knows for sure that all mutations will ultimately go via that method. Change testOverridingAdd to override the forcePut method not the put method to verify all this. (Or, even better, look at e.g. ListPropertyTest, where I've explicitly tested validation. Mea culpa on testOverridingAdd being a bit terrible :( )

thespags · 2020-03-08T22:19:52Z

Change testOverridingAdd to override the forcePut method not the put method to verify all this. (Or, even better, look at e.g. ListPropertyTest, where I've explicitly tested validation. Mea culpa on testOverridingAdd being a bit terrible :( )

I've updated to testOverridingAdd to forcePut. I'm a bit confused why entryRemainsUsableAfterCallingSetValue doesn't work. I'll admit I'm having a bit of a time trying to tie generated test code through generated prod code to the guava methods themselves to figure out what's going on.

Any more feedback for either entryRemainsUsableAfterCallingSetValue or the overall review? Thanks in advance.

alicederyn · 2020-03-13T15:00:44Z

If you dig into the logic of setValue in HashBiMap's entry implementations, it should become clear why entryRemainsUsableAfterCallingSetValue is not working. You'll need to rewrite CheckedBiMap's Entry implementation. Cleanest fix, given how BiMap works, is likely just to copy key + value from the underlying entry on construction, rather than keeping a link to the entry.

thespags · 2020-03-13T18:16:33Z

Cleanest fix, given how BiMap works, is likely just to copy key + value from the underlying entry on construction, rather than keeping a link to the entry.

Hi @alicederyn . I've updated to copy key and value from the underlying entry. As well as updating equals/hashcode to use those values.

thespags · 2020-03-16T23:31:18Z

@alicederyn sorry for the delay. I finally fixed the checkstyle issues. I wasn't sure how to suppress my comment line with a url, and checkstyle didn't like nested static method calls. Thank you.

thespags · 2020-03-26T21:45:01Z

Hi @alicederyn I was wondering when you would have a chance to go over my latest changes. Thank you!

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java

alicederyn · 2020-03-29T16:48:29Z

Sorry for the delay

thespags · 2020-03-30T18:33:44Z

src/test/java/org/inferred/freebuilder/processor/property/BiMapMutateMethodTest.java

+  @Test
+  public void callSetValueOnEntryChecksDuplicateValue() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("value already present: ");


I'm not sure for your framework the cleanest way to change a literal "beta" -> beta. I could do values.example(1).replace('\"', '') but I was thinking there was a better way.

You'd have to add that method to ElementFactory. exampleToString?

thespags · 2020-03-30T18:34:23Z

@alicederyn no worries.

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java

alicederyn · 2020-03-31T12:40:22Z

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java

+          .addLine("  }")
+          .addLine("")
+          .addLine("  @Override public V setValue(V value) {")
+          .addLine("    %s.checkArgument(!hasValue.test(value), \"value already present: \" + value);",


Use %s like in the put method

alicederyn · 2020-03-31T12:43:33Z

src/test/java/org/inferred/freebuilder/processor/property/BiMapMutateMethodTest.java

+  @Test
+  public void callSetValueOnEntryChecksDuplicateValue() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("value already present: ");


You'd have to add that method to ElementFactory. exampleToString?

thespags · 2020-03-31T16:50:26Z

You'd have to add that method to ElementFactory. exampleToString?

Done.

alicederyn · 2020-03-31T17:49:04Z

Build failed on checkstyle though: https://travis-ci.org/github/inferred/FreeBuilder/builds/669328480

Follows the behavoir of Maps under https://github.com/inferred/FreeBuilder#collections-and-maps

thespags · 2020-03-31T19:00:34Z

Thanks @alicederyn, I've fixed the offending lines and cleaned up my commits. Things are passing now.

alicederyn · 2020-04-04T18:24:36Z

Thanks @thespags. I merged a number of follow-up cleanups and bugfixes to this, and it is now released as v2.6.0.

thespags commented Feb 24, 2020

View reviewed changes

src/test/java/org/inferred/freebuilder/processor/property/BiMapPropertyTest.java Show resolved Hide resolved

thespags commented Feb 24, 2020

View reviewed changes

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java Show resolved Hide resolved

thespags requested a review from alicederyn February 24, 2020 07:06

alicederyn suggested changes Feb 24, 2020

View reviewed changes

alicederyn reviewed Mar 29, 2020

View reviewed changes

src/main/java/org/inferred/freebuilder/processor/excerpt/CheckedBiMap.java Show resolved Hide resolved

thespags commented Mar 30, 2020

View reviewed changes

alicederyn reviewed Mar 31, 2020

View reviewed changes

alicederyn approved these changes Mar 31, 2020

View reviewed changes

Add BiMap to the special support for collections and maps.

346e430

Follows the behavoir of Maps under https://github.com/inferred/FreeBuilder#collections-and-maps

alicederyn merged commit bbece66 into inferred:master Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BiMap to the special support for collections and maps. #419

Add BiMap to the special support for collections and maps. #419

thespags commented Feb 24, 2020

thespags commented Feb 24, 2020 •

edited

Loading

thespags Feb 24, 2020

alicederyn Feb 24, 2020

alicederyn left a comment

alicederyn Feb 24, 2020

alicederyn Feb 24, 2020

thespags Mar 1, 2020

alicederyn Mar 5, 2020

thespags commented Mar 1, 2020

alicederyn commented Mar 5, 2020

thespags commented Mar 8, 2020

alicederyn commented Mar 13, 2020

thespags commented Mar 13, 2020

thespags commented Mar 16, 2020

thespags commented Mar 26, 2020

alicederyn commented Mar 29, 2020

thespags Mar 30, 2020

alicederyn Mar 31, 2020

thespags commented Mar 30, 2020

alicederyn Mar 31, 2020

thespags Mar 31, 2020

alicederyn Mar 31, 2020

thespags commented Mar 31, 2020

alicederyn commented Mar 31, 2020

thespags commented Mar 31, 2020

alicederyn commented Apr 4, 2020

Add BiMap to the special support for collections and maps. #419

Add BiMap to the special support for collections and maps. #419

Conversation

thespags commented Feb 24, 2020

thespags commented Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicederyn left a 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

thespags commented Mar 1, 2020

alicederyn commented Mar 5, 2020

thespags commented Mar 8, 2020

alicederyn commented Mar 13, 2020

thespags commented Mar 13, 2020

thespags commented Mar 16, 2020

thespags commented Mar 26, 2020

alicederyn commented Mar 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thespags commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thespags commented Mar 31, 2020

alicederyn commented Mar 31, 2020

thespags commented Mar 31, 2020

alicederyn commented Apr 4, 2020

thespags commented Feb 24, 2020 •

edited

Loading