Conversation
|
@dwrensha assuming you're the right person to review this, but let me know if I should loop in others! I don't think I have permission to add reviewers or launch CI workflows |
|
Thanks for the PR. My main hesitation here is that this seems like a lot of duplication and could bloat the size of the generated files. Sometimes documentation for individual fields can span multiple paragraphs. (See e.g. this sandstorm schema.) If each such comment gets duplicated three times, it seems like things could get out of hand. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #665 +/- ##
===========================================
+ Coverage 51.64% 74.99% +23.35%
===========================================
Files 69 68 -1
Lines 33735 21057 -12678
===========================================
- Hits 17422 15792 -1630
+ Misses 16313 5265 -11048 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
I just removed the structure comments on Owned, Reader, Builder, and Pipeline, so now they're just attached to the associated Rust module. Not sure what to do about fields, though, because they are nice to see in the IDE on both the getters and setters and aren't tied to a module. What do you think? |
|
Hi @dwrensha, let me know if we can discuss anything or if there's anything else I can change! Appreciate the help. |
|
Just following up here @dwrensha if there's anything I can do to move this PR along! Apologies for bothering you about it; just wanted to try to contribute a useful change to Rust capnproto here that several of us at my company would love to see |
|
The 4x duplication for field doc comments is what makes me hesitate here. I wonder whether it would make sense to hoist the field comments into the module-level struct comment, so that they appear exactly once. And perhaps getters and setters could have comments that somehow link to the module-level comment. |
|
It's pretty common practice across other IDL code generators (like Protobuf in Java and Thrift) to intentionally duplicate field comments across every accessor to prioritize developer experience in IDEs. This duplication only lives in the autogenerated Rust files, which people generally should not check into repos and should not reference directly. These are also comments, which negligibly affect compiler speed compared to other AST elements. Bloat in non-autogenerated files would be a different story. I opt for prioritizing IDE hover support over DRYness in autogenerated build files. Indirection to other module-level comments will most likely make the IDE experience more challenging than necessary. |
First-time contributor to the repo here! Please feel free to be picky and let me know if there are any cultural or stylistic things to change!
This is a (hopefully) small change to read the source info from the schema and use it to generate rust doc strings, which should address #165 and #582.
Struct-level comments are added to the module
as well as Owned, Reader, Builder, and Pipeline structs. This is potentially overkill, but I thought it was kind of nice for tooltips in IDEs and at various levels of the generated cargo docs. Happy to debate it, though.Field-level comments are added to getters and setters in Builders and Readers. File-level comments are added at the top of the generated rust file (and are not included in cargo docs because
//!-type comments can easily break builds). Enum value comments are added before every value. Interface comments are added for both Client and Server methods. Constants feature comments before their definitions. Annotation comments are added to the module blocks.Also, I added .idea to the list of gitignored things for those that use RustRover like me.