New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Airbase 88 #11898

Merged
merged 6 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@electrum
Collaborator

electrum commented Nov 12, 2018

No description provided.

@electrum electrum force-pushed the electrum:airbase88 branch from 683d87e to efc0155 Nov 12, 2018

@electrum electrum force-pushed the electrum:airbase88 branch 2 times, most recently from 7cb3e7c to afe6344 Nov 12, 2018

@dain

dain approved these changes Nov 12, 2018

Minor comments... looks good assuming Travis passes.

Show resolved Hide resolved presto-main/src/test/java/com/facebook/presto/type/TestJsonOperators.java
PartitionUpdate actual = CODEC.fromJson(CODEC.toJson(expected));
assertEquals(actual.getName(), "test");

This comment has been minimized.

@dain

dain Nov 12, 2018

Contributor

instead of hardcoding the literals twice maybe just change these to call the getter on the expected... so assertEquals(actual.getName(), expected.getName());

This comment has been minimized.

@electrum

electrum Nov 12, 2018

Collaborator

I've actually given the opposite advice recently. This is more verbose, but makes sure we didn't screw up the constructor call ordering or assignment, and avoids copy/paste errors in the assertion calls.

(this would be less of a concern if we had a test to cover the constructor and getters directly)

@electrum electrum force-pushed the electrum:airbase88 branch from afe6344 to 96943d7 Nov 13, 2018

@electrum electrum force-pushed the electrum:airbase88 branch from 96943d7 to ff37640 Nov 14, 2018

@electrum electrum merged commit 7f9e06a into prestodb:master Nov 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@electrum electrum deleted the electrum:airbase88 branch Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment