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

GitHub Issue #38: truncate payload #167

Merged
merged 16 commits into from
May 16, 2017

Conversation

ArturMoczulski
Copy link
Contributor

Truncation is ready for review. This was implemented to resemble truncation strategies from the Ruby notifier as close as possible.

'environment' => 'tests'
));

$strategy = new FramesStrategy($databuilder);
Copy link
Contributor

Choose a reason for hiding this comment

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

s/databuilder/dataBuilder/

@rokob
Copy link
Contributor

rokob commented May 16, 2017

This looks very good, I'll give it another once over to see if we can get this merged today.

class FramesStrategy extends AbstractStrategy
{

const FRAMES_OPTIMIZATION_RANGE = 150;
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably comes from matching what the Gem does, but I feel like it is really large. Maybe we can make this 75 instead so that we get 150 total frames rather than 300 when we truncate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@rokob
Copy link
Contributor

rokob commented May 16, 2017

Tests pass, but lint doesn't like the long lines in the tests. Should be a simple fix then we can merge this and release 1.1

@ArturMoczulski
Copy link
Contributor Author

Okay, this is ready for merging

@rokob
Copy link
Contributor

rokob commented May 16, 2017

👍

@rokob rokob merged commit 26d611a into rollbar:master May 16, 2017
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.

None yet

2 participants