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

fix: Always trim source context lines #1443

Merged
merged 3 commits into from Apr 18, 2024
Merged

fix: Always trim source context lines #1443

merged 3 commits into from Apr 18, 2024

Conversation

loewenheim
Copy link
Contributor

The Python code always trims source lines. Since we don't have column numbers in jvm, we just pass 0 so the line is trimmed to 140 characters.

@loewenheim loewenheim requested a review from a team as a code owner April 18, 2024 11:29
@loewenheim loewenheim self-assigned this Apr 18, 2024
Swatinem
Swatinem approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I wonder if we might just want to bake this in as the default?

@loewenheim
Copy link
Contributor Author

I wonder if we might just want to bake this in as the default?

Yeah, me too. It's sort of a footgun.

@loewenheim loewenheim changed the title fix(proguard): Trim source context lines fix: Always trim source context lines Apr 18, 2024
@loewenheim
Copy link
Contributor Author

Baked the change into get_context_lines.

anonrig
anonrig requested changes Apr 18, 2024
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can you add a test please?

crates/symbolicator-service/src/source_context.rs Show resolved Hide resolved
@loewenheim loewenheim requested a review from anonrig April 18, 2024 13:55
@loewenheim loewenheim enabled auto-merge (squash) April 18, 2024 13:55
anonrig
anonrig approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

🚀

@loewenheim loewenheim merged commit 78ff1a2 into master Apr 18, 2024
10 checks passed
@loewenheim loewenheim deleted the proguard/trim-lines branch April 18, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swatinem Swatinem Swatinem approved these changes

@anonrig anonrig anonrig approved these changes

Assignees

@loewenheim loewenheim

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@loewenheim @Swatinem @anonrig