-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(grouping): java enhancer-by-guice wont group #22953
Conversation
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.
Thanks, @bruno-garcia for taking care of this. I understand how this can solve #21645, but I don't get it how this solves #18959, as this PR doesn't include a rule about GeneratedConstructorAccessorXX
.
@@ -61,3 +61,6 @@ family:native function:"?[[]SentryCrash *" -app -group | |||
family:native function:"?[[]SentryClient *" -app -group | |||
family:native function:"?[[]RNSentry *" -app -group | |||
family:native function:"?[[]SentrySDK *" -app -group | |||
|
|||
# Java | |||
family:other module:*EnhancerByGuice* -app -group |
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.
m
: I think it would be nice to add a comment on what EnhancerByGuice
actually is and which problem this rule solves. If I'm not mistaken it is generated by Google Guice.
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.
Literally pasting this string shows up what this is about though, not sure:
Generated code by Google Guice
is as useful as it is noise.
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.
Thanks, @bruno-garcia for taking care of this. I understand how this can solve #21645, but I don't get it how this solves #18959, as this PR doesn't include a rule about
GeneratedConstructorAccessorXX
.
This is a fair point, I'll add a rule for this too once @mitsuhiko validates the general approach and test.
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 agree that you can find out in a minute were EnhancerByGuice
comes from. Still, we have some information in the issues and also in this PR that is not obvious by this line of code. So you would have to do git blame
to find the corresponding PR and read the issue to find out why this rule is here.
@@ -61,3 +61,6 @@ family:native function:"?[[]SentryCrash *" -app -group | |||
family:native function:"?[[]SentryClient *" -app -group | |||
family:native function:"?[[]RNSentry *" -app -group | |||
family:native function:"?[[]SentrySDK *" -app -group | |||
|
|||
# Java | |||
family:other module:*EnhancerByGuice* -app -group |
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.
should we also ad *FastClassByGuice*
?
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 wasn't aware of this, good one, I'll add.
Create Family with Java and Groovy |
…nhancer-by-guice-grouping
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Closed in favor of: #23868 |
Added to the common ruleset as @mitsuhiko suggested #18959 (comment)
Note: Isn't
family:other
mean we'll apply this rule to all events of all platforms (other than what's covered bynative
andjavascript
)? If so should I add a new familyjava
and mapplatform=java
to it?Resolves #21645
Resolves: getsentry/sentry-java#1018
TODO:
Add in to resole: #18959
Also add
*$$FastClassByGuice$$*