Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

Conversation

@maciejwalkowiak
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Bind to SentryOption in Logback & Spring integrations instead of custom classes.

💡 Motivation and Context

After multiple try & errors with balancing between great user experience and ease of maintenance we came up with another option - just bind to SentryOption for the price of showing some irrelevant properties in Spring Boot application.properties autocompletion.

If it's decided to merge this one, we can close #528 and #529.

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

I believe this is the way to go.
One nit is the rename on the field (since now it's the only option not specifying the unit)

"sentry.dsn=http://key@localhost/proj",
"sentry.read-timeout-millis=10",
"sentry.shutdown-timeout-millis=20",
"sentry.shutdown-timeout=20",
Copy link
Member

Choose a reason for hiding this comment

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

M: Should we name it millis instead for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but then we need to rename getter and setter too.

Copy link
Contributor

@marandaneto marandaneto Aug 31, 2020

Choose a reason for hiding this comment

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

@maciejwalkowiak I'd rename the getters/setters to keep consistency, as all the android confs on the manifest ends with -millis as well.
we could keep the current getters/setters as aliases and annotate them with:

@ApiStatus.ScheduledForRemoval
@Deprecated

or maybe not even that, as this is a major bump.

@bruno-garcia bruno-garcia merged commit 2d6e215 into feat/sentry-java Aug 29, 2020
@bruno-garcia bruno-garcia deleted the refactor-binding-options branch August 29, 2020 20:14
maciejwalkowiak added a commit to maciejwalkowiak/sentry-android that referenced this pull request Aug 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants