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

String escaping fix #580

Merged
merged 4 commits into from
Oct 31, 2023
Merged

String escaping fix #580

merged 4 commits into from
Oct 31, 2023

Conversation

japersik
Copy link
Contributor

@jhump
Copy link
Owner

jhump commented Oct 31, 2023

Oof, great catch! What an embarrassing bug.

What's missing from this pull request is updates to tests to catch this sort of thing in the future. The best way to is to edit test files in internal/testprotos and then to re-generate the associated "golden" output files to make sure they are now correct.

I went ahead and did this, because I noticed that you checked the box so that maintainers are allowed to edit this pull request. However, it looks like you opened this PR from the main branch of your fork, and I am unable to push any commits to that branch.

I think you'll need to apply this diff to your fork to improve test coverage for all escapes, and then I can merge this pull request once you've pushed those:
japersik/protoreflect@main...jhump:japersik-pr

@japersik
Copy link
Contributor Author

japersik commented Oct 31, 2023

I think you'll need to apply this diff to your fork to improve test coverage for all escapes

Done

@jhump jhump merged commit e429ee6 into jhump:main Oct 31, 2023
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.

2 participants