ref: sentry-core changes for console app#473
Conversation
bruno-garcia
left a comment
There was a problem hiding this comment.
It looks like the right direction. Are we far from dropping the draft flag?
It's getting pretty daunting to review this, I hope we can merge soon
interested to learn why it's getting |
bruno-garcia
left a comment
There was a problem hiding this comment.
This is just a draft and it looks huge:
Probably just 20% of that is actually relevant to the change. Please let's avoid mixing a bunch of refactor in PRs that add a feature or change a core part of the SDK.
| @@ -1,43 +1,56 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" | |||
| xmlns:tools="http://schemas.android.com/tools" | |||
There was a problem hiding this comment.
What changed here? Not sure what to review
There was a problem hiding this comment.
probably changed by the IDE automatically and will be reverted, Draft PR
while I agree with the comment,
while I agree with this statement, we agreed to lift a draft PR to share the knowledge of the session data generator (how we did it), this was asked by you btw on a daily standup. the only code that actually needed to land in the master branch was #474 which is a separate PR, tested, and merged. all these changes came for free because I actually needed them to make sentry-core to work standalone. Let's please not put a lot of useless comments in a Draft PR and focus on the specific bits mentioned in the daily call, which is the changes from sentry-core that actually is gonna be needed for the new sentry-java, all the rest will be discarded, but if I didn't push it, you won't know how I generated the session data. I'm trying to improve things here, and trying to reuse some work for the future sentry-java, this PR is just early feedback of "how I would fix sentry-core to be usable in sentry-java", so adding negative/useless comments just makes it harder to continue. happy to discuss offline as well. |
I wasn't aware they'd come into the same PR. I was genuinely askign why that code is in this PR, since I didn't know if the plan was to merge both things or not, at the same time. Reason why I suggested we don't do that.
I understand and appreciate the work you put into this and the constant improvement of things. I wasn't trying to add any "negative/useless" comment, simply reiterating what we discussed in several meetings, to try to avoid doing different types of changes (refactor, different features, etc) in a single PR since it can get really difficult to review things.
The title is what made it confusing because I thought it was simply the change, as the title says sentry-core changes for console app so all the other code seem not to fit here. We can catch up offline, no problem. I'm OOO until Tuesday though, sorry. |
|
|
||
| public SentryAndroidOptions() { | ||
| setSentryClientName(BuildConfig.SENTRY_CLIENT_NAME + "/" + BuildConfig.VERSION_NAME); | ||
| setSdkInfo(SdkInfo.createSdkInfo(BuildConfig.SENTRY_CLIENT_NAME, BuildConfig.VERSION_NAME)); |
There was a problem hiding this comment.
This should be an override to SentryOptions ctor which sets it to sentry.java
📢 Type of change
📜 Description
ref: sentry-core changes for console app
💡 Motivation and Context
it's not possible to use sentry-core standalone, this is a first PR that makes things a bit easier.
💚 How did you test it?
📝 Checklist
🔮 Next steps