Refactor using common parts of SentryOptions.
#528
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
maciejwalkowiak
changed the title
SentryOptions usage.SentryOptions.
| private Boolean debug; | ||
|
|
||
| /** @see SentryOptions#diagnosticLevel */ | ||
| private SentryLevel diagnosticLevel = SentryLevel.DEBUG; |
There was a problem hiding this comment.
can you use SentryOptions.DEFAULT_DIAGNOSTIC_LEVEL here?
There was a problem hiding this comment.
It should be null by default. My bad.
There was a problem hiding this comment.
It should be DEBUG by default (perhaps with a const DEFAULT_DIAGNOSTIC_LEVEL) though, right?
If the user sets option.setDebug(true) it should automatically start printing all diag msgs of all levels
| /** @see SentryOptions#shutdownTimeoutMillis */ | ||
| private Long shutdownTimeoutMillis; | ||
|
|
||
| /** @see SentryOptions#flushTimeoutMillis */ | ||
| private Long flushTimeoutMillis; | ||
|
|
||
| /** @see SentryOptions#readTimeoutMillis */ | ||
| private Integer readTimeoutMillis; | ||
|
|
||
| /** @see SentryOptions#bypassSecurity */ | ||
| private Boolean bypassSecurity; | ||
|
|
||
| /** @see SentryOptions#debug */ | ||
| private Boolean debug; |
There was a problem hiding this comment.
should we use primitive types? otherwise, we'd need to do NPE checks in the setters too, also for all the other fields that are primitive types in SentryOptions
There was a problem hiding this comment.
The advantage is being able to know if the value was set explicitly by the user and being able to flip the default after invoking a user callback.
But for this field I agree probably doesn't make sense
There was a problem hiding this comment.
As @bruno-garcia said, we need to know which values were set explicitly by user. This way we don't need to keep in sync default values on SentryOptions and SentryCommonOptions. This also includes data type for fields - they have to be nullable otherwise we loose this information.
| } | ||
|
|
||
| public void setDiagnosticLevel(SentryLevel diagnosticLevel) { | ||
| this.diagnosticLevel = diagnosticLevel; |
There was a problem hiding this comment.
if you look at SentryOptions.setDiagnosticLevel you see that we don't allow null values, and here it is possible, if one sets it to null, it'll crash the app.
There was a problem hiding this comment.
In applyTo there is a check for null. The idea is that we apply only properties explicitly set by user.
@marandaneto I don't see how I can apply inheritance here to achieve this. |
|
Superseded by #530 |
3 participants
📢 Type of change
📜 Description
Use common subset of SentryOptions in all 3rd party integrations to avoid code duplication and possibility to miss support of newly added properties in those integrations.
💡 Motivation and Context
Duplication between Logback and Spring Boot integration.
📝 Checklist