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.

Sign up for GitHub

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

Jump to bottom

Add raise to sorbet metrics #273

Closed
wants to merge 1 commit into from
Closed

Add raise to sorbet metrics #273

wants to merge 1 commit into from
+775 −35

Conversation

bitwise-aiden
Copy link
Contributor

Description

Closes: #259

This PR updates #srb_metrics to raise an error based on the output captured from the call to Sorbet.

I've also removed capture_err from #srb_metrics since the method doesn't return the exec result, so to my understanding the caller doesn't need to be opinionated about how it's called.
This has also meant that it is possible to capture the specific error message and bubble it up to the user.

What should reviewers focus on?

  • Do we want to raise the error message directly from Sorbet?
  • Any objections to removing capture_err from the #srb_metrics method?
  • Do we want to wrap the error in coverage to return something nicer to the user?
    • Currently it will bubble the error message produced by Sorbet.

@bitwise-aiden bitwise-aiden requested a review from a team as a code owner January 13, 2023 20:11
Morriar
Morriar reviewed Jan 16, 2023
View reviewed changes
@bitwise-aiden bitwise-aiden force-pushed the shopify-aiden/259/add-metrics-raise branch from dd4c80d to 6db4ff6 Compare January 17, 2023 20:41
@bitwise-aiden bitwise-aiden mentioned this pull request Jan 17, 2023
Closed
@bitwise-aiden bitwise-aiden force-pushed the shopify-aiden/259/add-metrics-raise branch from e0ab16b to 3fd3bea Compare January 17, 2023 23:09
@bitwise-aiden bitwise-aiden requested a review from Morriar January 17, 2023 23:10
Morriar
Morriar reviewed Jan 17, 2023
View reviewed changes
Copy link
Collaborator

@Morriar Morriar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One comment about the structure of these changes.

I think this PR can actually be cut into 3 smaller, easier to read PRs:

  1. The Gemfile + RBI changes
  2. The refactor around segfault (and related changes to the run and bump commands)
  3. The new feature for killed

@bitwise-aiden
Add raise to sorbet caller
Loading
Loading status checks…
f39759b
@bitwise-aiden bitwise-aiden force-pushed the shopify-aiden/259/add-metrics-raise branch from 3fd3bea to f39759b Compare January 17, 2023 23:11
@bitwise-aiden
Copy link
Contributor Author

I'll split things out into their own PRs tomorrow

@bitwise-aiden
Copy link
Contributor Author

bitwise-aiden commented Jan 18, 2023 •
edited
Loading

Split out into the 3 PRs requested, here:

Closing this one to avoid future confusion

@greatscotty greatscotty deleted the shopify-aiden/259/add-metrics-raise branch February 9, 2024 17:19
@Morriar Morriar added the enhancement New feature or request label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morriar Morriar

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spoom should raise if it can't produce snapshot metrics
2 participants
@bitwise-aiden @Morriar