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 joda to 2.10 #11890

Merged
merged 1 commit into from Nov 13, 2018

Conversation

Projects
None yet
6 participants
@haozhun
Contributor

haozhun commented Nov 10, 2018

The tzdata in joda 2.10 matches that of Java 10.0.2 and 8u181

@@ -51,6 +51,7 @@
<dep.okhttp.version>3.9.0</dep.okhttp.version>
<dep.jdbi3.version>3.4.0</dep.jdbi3.version>
<dep.drift.version>1.14</dep.drift.version>
<dep.joda.version>2.10</dep.joda.version>

This comment has been minimized.

@hellium01

hellium01 Nov 10, 2018

Contributor

Should we update in airbase instead?

@findepi

This comment has been minimized.

Member

findepi commented Nov 10, 2018

Travis seems to complain

Caused by: java.lang.NoClassDefFoundError: Could not initialize class com.facebook.presto.util.DateTimeZoneIndex

@haozhun haozhun force-pushed the haozhun:joda branch from d89c3a4 to d8e0955 Nov 12, 2018

@electrum

@dain should also review

switch (i) {
case 2040: // previous spot for Canada/East-Saskatchewan
continue;
default:

This comment has been minimized.

@electrum

electrum Nov 13, 2018

Collaborator

This seems strange. Why not remove it?

This comment has been minimized.

@haozhun

haozhun Nov 13, 2018

Contributor

I don't feel strongly. But I believe switch should always have default.

  • If the cases are complete, it is important that default throws.
  • If the cases are incomplete, default gives clear indication that it's not complete.

This comment has been minimized.

@dain

dain Nov 13, 2018

Contributor

How about being even more specific... before the loop add:

// previous spot for Canada/East-Saskatchewan
assertFalse(hasValue[2040]);
hasValue[2040] = true;

Also maybe rename hasValue to something like validTimeZoneKeys

continue;
default:
// do nothing
break;

This comment has been minimized.

@electrum

electrum Nov 13, 2018

Collaborator

Having a break here is confusing, since the break is for the switch, but the continue is for the loop.

This comment has been minimized.

@haozhun

haozhun Nov 13, 2018

Contributor

removed

@electrum electrum requested a review from dain Nov 13, 2018

@haozhun haozhun force-pushed the haozhun:joda branch from d8e0955 to 90531df Nov 13, 2018

@dain

dain approved these changes Nov 13, 2018

One comment but otherwise looks good.

switch (i) {
case 2040: // previous spot for Canada/East-Saskatchewan
continue;
default:

This comment has been minimized.

@dain

dain Nov 13, 2018

Contributor

How about being even more specific... before the loop add:

// previous spot for Canada/East-Saskatchewan
assertFalse(hasValue[2040]);
hasValue[2040] = true;

Also maybe rename hasValue to something like validTimeZoneKeys

Update joda to 2.10
The tzdata in joda 2.10 matches that of Java 10.0.2 and 8u181

@haozhun haozhun force-pushed the haozhun:joda branch from 90531df to e5b1c74 Nov 13, 2018

@haozhun haozhun merged commit e5b1c74 into prestodb:master Nov 13, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment