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

Add time zone conversion capability which fixes #424 #488

Merged
merged 2 commits into from Feb 10, 2016

Conversation

4 participants
@ocadaruma
Contributor

ocadaruma commented Feb 6, 2016

I've reimplemented time zone conversion via adding field to some classes and proxying ResultSet. (as suggested in scalikejdbc/ja gitter channel.)
(previous PR is #478)

This PR brings binary incompatibility.

seratch added a commit to seratch/scalikejdbc that referenced this pull request Feb 8, 2016

seratch added a commit to seratch/scalikejdbc that referenced this pull request Feb 8, 2016

@seratch

This comment has been minimized.

Show comment
Hide comment
@seratch

seratch Feb 8, 2016

Member

@ocadaruma Thank you for your contribution. Please check my suggestions here: ocadaruma#1

Member

seratch commented Feb 8, 2016

@ocadaruma Thank you for your contribution. Please check my suggestions here: ocadaruma#1

@ocadaruma

This comment has been minimized.

Show comment
Hide comment
@ocadaruma

ocadaruma Feb 8, 2016

Contributor

@seratch Thank you for suggestion.
I merged seratch:ocadaruma-feature/timezone-conversion

Contributor

ocadaruma commented Feb 8, 2016

@seratch Thank you for suggestion.
I merged seratch:ocadaruma-feature/timezone-conversion

ocadaruma added a commit to ocadaruma/scalikejdbc that referenced this pull request Feb 8, 2016

@seratch

This comment has been minimized.

Show comment
Hide comment
@seratch

seratch Feb 8, 2016

Member

@ocadaruma Sorry for bothering you. Would you mind rebasing this?

This PR is already fine enough for me. If you have some comments, please check this PR within two days. >All

Member

seratch commented Feb 8, 2016

@ocadaruma Sorry for bothering you. Would you mind rebasing this?

This PR is already fine enough for me. If you have some comments, please check this PR within two days. >All

@@ -10,5 +10,6 @@ case class ConnectionPoolSettings(
validationQuery: String = null,
connectionPoolFactoryName: String = null,
driverName: String = null,
warmUpTime: Long = 100L)
warmUpTime: Long = 100L,
timeZone: String = null)

This comment has been minimized.

@tkawachi

tkawachi Feb 8, 2016

Member

Why not using Option[String] here? Though I don't know why other fields are also nullable. @seratch

@tkawachi

tkawachi Feb 8, 2016

Member

Why not using Option[String] here? Though I don't know why other fields are also nullable. @seratch

This comment has been minimized.

@seratch

seratch Feb 8, 2016

Member

@tkawachi In my opinion, using Option here doesn't have any meaning in pactice.
(e.g.) ConnectionPoolSettings(driverName = Some("com.mysql.jdbc.Driver")) instead of ConnectionPoolSettings(driverName = "com.mysql.jdbc.Driver") is really better?

@seratch

seratch Feb 8, 2016

Member

@tkawachi In my opinion, using Option here doesn't have any meaning in pactice.
(e.g.) ConnectionPoolSettings(driverName = Some("com.mysql.jdbc.Driver")) instead of ConnectionPoolSettings(driverName = "com.mysql.jdbc.Driver") is really better?

This comment has been minimized.

@tkawachi

tkawachi Feb 8, 2016

Member

I don't understand your point. Do you mean nullable fields are easier to construct because it requires fewer key strokes?

I searched timeZone field usage, and found 2 occurrences. One is to construct ConnectionPoolSettings from a config

timeZone = configMap.get("timeZone").orNull[String]

and Another is getting timeZone from a settings

    val timeZoneSettings = Option(settings.timeZone).fold(TimeZoneSettings()) { timeZone =>
      TimeZoneSettings(true, java.util.TimeZone.getTimeZone(timeZone))
    }

I felt that Option[String] is better by just looking these two. At the second occurrence, we might code wrongly if we forget it's nullable.

@tkawachi

tkawachi Feb 8, 2016

Member

I don't understand your point. Do you mean nullable fields are easier to construct because it requires fewer key strokes?

I searched timeZone field usage, and found 2 occurrences. One is to construct ConnectionPoolSettings from a config

timeZone = configMap.get("timeZone").orNull[String]

and Another is getting timeZone from a settings

    val timeZoneSettings = Option(settings.timeZone).fold(TimeZoneSettings()) { timeZone =>
      TimeZoneSettings(true, java.util.TimeZone.getTimeZone(timeZone))
    }

I felt that Option[String] is better by just looking these two. At the second occurrence, we might code wrongly if we forget it's nullable.

This comment has been minimized.

@seratch

seratch Feb 8, 2016

Member

@tkawachi I can understand your opinion in general, but let's go with nullable String here to keep API compatibility with validationQuery, connectionPoolFactoryName and driverName.

@seratch

seratch Feb 8, 2016

Member

@tkawachi I can understand your opinion in general, but let's go with nullable String here to keep API compatibility with validationQuery, connectionPoolFactoryName and driverName.

@ocadaruma

This comment has been minimized.

Show comment
Hide comment
@ocadaruma

ocadaruma Feb 8, 2016

Contributor

@seratch OK. What should I do about MimaSettings.scala ? (Should I discard the commit or squash ?)

Contributor

ocadaruma commented Feb 8, 2016

@seratch OK. What should I do about MimaSettings.scala ? (Should I discard the commit or squash ?)

@seratch

This comment has been minimized.

Show comment
Hide comment
@seratch

seratch Feb 8, 2016

Member

@ocadaruma My change on MimaSettings.scala is required. Please just do git rebase --pull {scalikejdbc remote} master.

Member

seratch commented Feb 8, 2016

@ocadaruma My change on MimaSettings.scala is required. Please just do git rebase --pull {scalikejdbc remote} master.

seratch added a commit that referenced this pull request Feb 10, 2016

Merge pull request #488 from ocadaruma/feature/timezone-conversion
Add time zone conversion capability which fixes #424

@seratch seratch merged commit 5bd9bf2 into scalikejdbc:master Feb 10, 2016

1 check passed

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