-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Move codegen into src/
folder
#3333
Conversation
I would love to see code completion working with protobuf generated code. That has been frustrating not being able to do that. The only concern I have with this change is that |
Codecov Report
@@ Coverage Diff @@
## master #3333 +/- ##
=======================================
Coverage 85.48% 85.48%
=======================================
Files 294 294
Lines 54115 54100 -15
=======================================
- Hits 46259 46248 -11
+ Misses 7856 7852 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@tustvold @alamb What do you two think? This seems (to me) to be the best of all worlds:
The only downside is that |
I will review this in more detail, likely tomorrow morning.
…On Thu, Sep 1, 2022 at 5:11 PM Brent Gardner ***@***.***> wrote:
@tustvold <https://github.com/tustvold> @alamb <https://github.com/alamb>
What do you two think? This seems (to me) to be the best of all worlds:
1. generated code is not checked in
2. generated code is in source folders so IDEs work again
The only downside is that protoc is required, but that was already a
requirement for datafusion. If anyone is interested I can (in another
PR), have build.rs download and unzip the appropriate protoc for the
platform.
—
Reply to this email directly, view it on GitHub
<#3333 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADXZMOFBEEMKESLDLOXYOTV4ELW3ANCNFSM6AAAAAAQCTKVW4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
36e7793
to
6028fde
Compare
Sorry I ran out of time today to give this a careful look. It is on my list... There was all sorts of good stuff happening in DataFusion I still haven't fully caught up |
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.
I like this idea and I think we should try it. If something about generating the code into the source tree causes unforseen issues we can always roll this back.
Thanks a lot @avantgardnerio
@@ -277,7 +277,9 @@ jobs: | |||
rustup default stable | |||
rustup component add rustfmt | |||
- name: Run | |||
run: ci/scripts/rust_fmt.sh | |||
run: | | |||
echo '' > datafusion/proto/src/generated/datafusion.rs |
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.
This is needed so it works on a clean checkout, right?
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.
The CI jobs are parallelized, so Lint
doesn't get a copy of the files generated by Build
, but lint did complain I did a mod datafusion
for a file that didn't exist. Since this is a CI-only issue, this seemed like an acceptable fix for CI ... ?
This PR appears to need a |
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
3950da7
to
8ffc83f
Compare
I believe these changes were also included in #3311 so closing this PR -- @avantgardnerio please let me know if that is incorrect and reopen |
Which issue does this PR close?
Closes #3332.
Rationale for this change
Described in issue.
What changes are included in this PR?
A change to the generated code location.
Are there any user-facing changes?
no