Skip to content
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

Strip path prefixes on profile frames #1568

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Conversation

stayallive
Copy link
Collaborator

This was an open TODO that is resolved!

@ste93cry
Copy link
Collaborator

Wouldn't be better to make PrefixStripper a final class with a static method accepting the list of prefixes instead of using a trait?

@stayallive
Copy link
Collaborator Author

That could also work yes. I don't think there is a technical reason why either way is "better" so potato, potahto really in this case 😄

Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

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

A quick test run with some app that reports to https://sentry.io would be appreciated, besides that, :shipit:

tests/Profiling/ProfileTest.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Collaborator

ste93cry commented Jul 13, 2023

That could also work yes. I don't think there is a technical reason why either way is "better"

Some of the reasons I can think of is that:

  • Even if not impossible, they are more difficult to test than a class or a method
  • They introduce horizontal inheritance, so understanding a code base can be significantly harder. Injecting a dependency is a lot cleaner in this aspect, as you make it clear what a class needs to work. But in this case, a static helper method would probably be enough
  • Since a trait is basically a way to copy-paste some code in multiple places, you are effectively coupling it with the classes it will be used in, risking to make some change in one that doesn't suit the others. In this case the method does not access anything on $this, but you never know in the future what could happen.

Lately I've seen a lot of discussions around traits and why people think they are either good or bad. Honestly I don't have a real strong opinion on this, and in this case I agree that it doesn't really matter probably, but if it was me I would have created an helper class. Also, I don't like passing around all the client config when the function really needs just a plain array of strings. Also, the method is not static (so it can access the instance properties), but it takes the options from its arguments 🤔

If you really want to leave it as-is, I would at least add the Trait suffix to the trait to make it explicit what is it.

@stayallive
Copy link
Collaborator Author

For the requested sentry.io test (using a Laravel application):

Before:

image

After:

image

Looks a lot better to me 😎

@stayallive stayallive merged commit 15a028d into master Jul 26, 2023
28 of 29 checks passed
@stayallive stayallive deleted the strip-prefixes-profile branch July 26, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants