Skip to content
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 lenientString to SqlRow #84

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Add lenientString to SqlRow #84

merged 1 commit into from
Sep 27, 2023

Conversation

tmccombs
Copy link
Contributor

And use it for ColReader to restore previous behaviour

* Like [[stringOption]], but calls `getString` instead of `getObject`, so may
* convert other types into Strings, depending on the driver.
*/
def lenientStringOption(column: String): Option[String] = getResultSetOption(resultSet.getString(column))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as strictString and strictStringOption?

@tmccombs tmccombs force-pushed the lenient-string branch 3 times, most recently from 01676c1 to 1eaab4b Compare September 27, 2023 20:10
Copy link
Contributor

@richard-shurtz richard-shurtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like flipping those back is a more consistent solution. Everything else looks good

def strictString(column: String): String = resultSet.getString(column)
def strictStringOption(column: String): Option[String] = getResultSetOption(resultSet.getString(column))
def strictString(column: String): String = string(column)
def strictStringOption(column: String): Option[String] = stringOption(column)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd flip this personally. Have stringOption call through to strictStringOption rather than the other way around.
If 'strict' represents "matching the behavior from the ResultSet directly", and 'standard' methods represent 'the interface we want', then it makes sense for strictString to call the resultSet, and it make sense of stringOption to say "the behavior we want matches the ResultSet behavior"

@@ -73,8 +73,8 @@ class SqlRow(val resultSet: java.sql.ResultSet) extends ResultSetWrapper {
def strictShortOption(column: String): Option[Short] = getResultSetOption(resultSet.getShort(column))
def strictSQLXML(column: String): SQLXML = resultSet.getSQLXML(column)
def strictSQLXMLOption(column: String): Option[SQLXML] = getResultSetOption(resultSet.getSQLXML(column))
def strictString(column: String): String = resultSet.getString(column)
def strictStringOption(column: String): Option[String] = getResultSetOption(resultSet.getString(column))
def strictString(column: String): String = string(column)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are changing strictString to get the "throw NoSuchElementExceptionbehavior, rather thannullor0`. Why aren't we changing strictLong as well? (Again, this is why I'd 'flip' the implementation of strictString and strictStringOption back)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant to undo that change.

Instead of calling getObject and inspecting the type.

This makes the parsers more consistent. And restores the previous behavior of ColReaders.

This makes it so longOption, bigDecimalOption, and stringOption, etc. may not throw exceptions in
cases where it did before. But I think this is probably the behavior that would be expected.
@tmccombs tmccombs merged commit fae9766 into master Sep 27, 2023
3 checks passed
@tmccombs tmccombs deleted the lenient-string branch September 27, 2023 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants