Skip to content

Conversation

@Swatinem
Copy link
Contributor

The synchronization fixes have been proposed in #172 originally

@Swatinem Swatinem requested a review from a team September 14, 2020 10:20
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

@Swatinem I assume that this makes #259 redundant?

@Swatinem
Copy link
Contributor Author

@Swatinem I assume that this makes #259 redundant?

yes. And I updated your comments about the response log.

Comment on lines +255 to +256
Err(err) => { sentry_debug!("Failed to read sentry response: {}", err); },
Ok(text) => { sentry_debug!("Get response: `{}`", text); },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Err(err) => { sentry_debug!("Failed to read sentry response: {}", err); },
Ok(text) => { sentry_debug!("Get response: `{}`", text); },
Err(err) => sentry_debug!("Failed to read sentry response: {}", err),
Ok(text) => sentry_debug!("Get response: `{}`", text),

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering how this does not fail rustfmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its inside the macro, so none of that stuff is rustfmt-ed, also, I needed to wrap this in a block because of the way the sentry_debug macro is built :-(

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks. At least the comment after the } should go usually.

@Swatinem Swatinem merged commit 6bfb54b into master Sep 14, 2020
@Swatinem Swatinem deleted the fix/transport-fixes branch September 14, 2020 14:40
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