-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(native): Generation of thrift protocol #26419
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefine the Thrift-to-JSON preprocessing to correctly strip the “drift.recursive_reference = true” declaration via an improved regex and ensure the CI setup installs the required Python thrift parser dependency. Class diagram for updated Thrift preprocessing functionclassDiagram
class preprocess {
+preprocess(file_path, output_temp_path)
-Improved regex to remove "drift.recursive_reference = true"
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
dfb1c2b to
a2b663d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Precompile the regex patterns outside the loop in thrift2json.py to avoid recompiling them on each line and improve performance.
- Tighten the regex for removing drift.recursive_reference (for example with word boundaries) to avoid accidentally stripping similar tokens.
- Verify that the new pip package 'ptsd-jbroll' is the intended thrift parser and consider pinning its version to prevent future compatibility issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Precompile the regex patterns outside the loop in thrift2json.py to avoid recompiling them on each line and improve performance.
- Tighten the regex for removing drift.recursive_reference (for example with word boundaries) to avoid accidentally stripping similar tokens.
- Verify that the new pip package 'ptsd-jbroll' is the intended thrift parser and consider pinning its version to prevent future compatibility issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a2b663d to
73a9093
Compare
The thrift generation was broken because a line that was expected to be removed was not and caused a thrift parse error. That line is "drift.recursive_reference = true,” which was not caught by the regex attempting to remove it. This PR also adds the required thrift parser to the python installation of the dependency to use it in the CI.
73a9093 to
c243388
Compare
|
thank you for fixing this! |
|
@tdcmeehan Please review when you get a chance. |
tdcmeehan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ptsd-jbroll is not maintained. I think we need to come up with a policy on introducing unmaintained dependencies.
The thrift generation was broken because a line
that was expected to be removed was not and caused a thrift parse error.
That line is "drift.recursive_reference = true,” which was not caught by the regex attempting to remove it.
This PR also adds the required thrift parser to the python installation of the dependency to use it in the CI.
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.