Skip to content

Conversation

vaind
Copy link
Contributor

@vaind vaind commented Mar 9, 2025

We're adding a new SDK for Nintendo Switch & it requires that the attachment sent on crashes is parsed and expanded into the outer envelope.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@vaind vaind marked this pull request as ready for review March 12, 2025 18:49
@vaind vaind requested a review from a team as a code owner March 12, 2025 18:49
@vaind vaind requested a review from Dav1dde March 12, 2025 18:49
Copy link
Contributor

@iambriccardo iambriccardo left a comment

Choose a reason for hiding this comment

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

Left some comments

@@ -0,0 +1,390 @@
//! Nintendo Switch processor related code.
//!
//! These functions are included only in the processing mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you maybe add better docs on this module to explain what is the dying message and why it is used, might be helpful to future readers.

Copy link
Contributor Author

@vaind vaind Mar 13, 2025

Choose a reason for hiding this comment

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

I've just changed the first line of the expand() function header to say /// Expands Nintendo Switch crash-reports.. I don't believe we can add more details since this information is not public.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to make the dying message format public, either so we can link to it from here, or actually describe the format in Relay.

Any change to the format made must be first made in Relay anyways.

Co-authored-by: Riccardo Busetti <[email protected]>
@Dav1dde
Copy link
Member

Dav1dde commented Mar 13, 2025

Haven't taken a look at the changes yet, do we have an example 'real' dying message we can use for an integration test?

We should definitely have at least one integration test which asserts Relay does the right thing (preferably more).

@vaind
Copy link
Contributor Author

vaind commented Mar 16, 2025

do we have an example 'real' dying message we can use for an integration test?

not yet, i'm working on the SDK implementation in parallel and will update once I have one.

We should definitely have at least one integration test which asserts Relay does the right thing (preferably more).

where would these go in the repo?

@Dav1dde
Copy link
Member

Dav1dde commented Mar 17, 2025

where would these go in the repo?

You can put them into tests/integration/test_switch.py similar to tests/integration/test_unreal.py.

@vaind
Copy link
Contributor Author

vaind commented Apr 7, 2025

@Dav1dde I've added the integration test. Do you mind having another look at the PR?

@@ -0,0 +1,390 @@
//! Nintendo Switch processor related code.
//!
//! These functions are included only in the processing mode.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to make the dying message format public, either so we can link to it from here, or actually describe the format in Relay.

Any change to the format made must be first made in Relay anyways.

@vaind vaind requested a review from Dav1dde April 11, 2025 12:30
@Dav1dde Dav1dde enabled auto-merge (squash) April 14, 2025 06:56
@Dav1dde Dav1dde disabled auto-merge April 14, 2025 11:39
@Dav1dde Dav1dde merged commit cecd8dd into getsentry:master Apr 16, 2025
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants