Make SentryOptions inherit from SentryCommonOptions #529
Conversation
…n to copy values.
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
| * background queue and this queue is given a certain amount to drain pending events Default is | ||
| * 2000 = 2s | ||
| */ | ||
| private long shutdownTimeout = 2000; // 2s |
There was a problem hiding this comment.
I wouldn't rename any of the options not to bring any breaking changes here.
Also, I'd suggest we're always explicit on the unit. Since this is long, this timeout is in Seconds? MIlliseconds?
Joshua Bloch talks about that in this video which I highly recommend: https://www.youtube.com/watch?v=heh4OeB9A-c
There was a problem hiding this comment.
I agree. It got renamed because there was a mismatch between setters/getters and the field name, thus it didn't comply to Java Beans convention making it impossible to use with Spring @ConfigurationProperties. Either we rename the field (as it's done in this PR) and do not introduce breaking changes (as the setters & getters do not change), or follow Joshua Bloch suggestion and rename setters and getters.
|
Superseded by #530 |
📢 Type of change
📜 Description
Refactoring for #528
Make SentryOptions inherit from SentryCommonOptions and use reflection to copy values.
💡 Motivation and Context
Thanks to using reflection and inheritance we limit even more number of places that have to be modified when new properties are added.
💚 How did you test it?
Existing unit & integration tests cover reflection usage.
📝 Checklist
🔮 Next steps
The question is if it's the way to go or @bruno-garcia you had something different in mind?