-
Notifications
You must be signed in to change notification settings - Fork 66
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
Catching Error too in isOpenApiStr
#490
Conversation
@@ -8,7 +8,7 @@ import java.net.URL | |||
public fun isOpenApiStr(text: String): Boolean = try { | |||
val parsed = OpenAPIParser().readContents(text, null, null) | |||
parsed.openAPI?.components?.schemas != null | |||
} catch (_: Exception) { | |||
} catch (_: Throwable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking, could we give more info here, just logging the reason of Error or Exception, now it silently happened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, if logger is not setup, create a TODO with "add logger"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we already have the problem of the openAPI parser logging exceptions each time we read anything with DataFrame. I don't want to needlessly log even more stuff in this case. That said, adding a logger at level INFO or DEBUG might still be beneficial indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sad about openAPI parser logging during reading, Debug level will be a good option, so, probably a good idea to add to-do or comment, to this issue or to the openAPI parser logging exception issue (if it exists), to explain why we doing it silently, because for me, it looks like antipattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent Throwable
Also cherry picked into https://github.com/Kotlin/dataframe/tree/0.12.1 |
Fixes #489