feat: generate martian struct field descriptions and filenames #480
Conversation
pjedge
commented
Mar 5, 2024
•
pjedge
commented
- When deriving MartianStruct for a Rust struct convert doc comments into Martian struct field descriptions (optional 3rd column of martian struct field)
- Allow direct file naming (optional 4th column in Martian struct def) with a new #[mro_filename = "my_filename"] attribute.
|
@adam-azarchs curious if you can recommend a different name for |
file (mro_rename attribute).
pjedge
force-pushed
the
pe/implement-struct-descriptions-and-renames
branch
from
775e452 to
33117be
Compare
martian/src/mro.rs
Outdated
| fn name_width(&self) -> usize { | ||
| if self.mro_filename.is_none() && self.desc.is_none() { | ||
| self.name.len() + 1 // string length + 1 space | ||
| } else { | ||
| self.name.len() // string_length | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure it makes sense to be reimplementing this logic rather than just relying on passing the output through mro format. For one thing, this won't actually match the logic used by mro format, which is a bit more complex:
https://github.com/martian-lang/martian/blob/6171ed9de74fad39566b2f08685dcb86d5949eff/martian/syntax/format_callable.go#L116-L121
That is, it does not consider the lengths of names > 35 characters in length, or help texts > 25 characters in length.
There was a problem hiding this comment.
I suppose we could generate unformatted code and then pass things through MRO format. I know in the past I've had failing GHA tests because the generated MRO code in the repo gets formatted by MRO format and then doesn't match the directly generated code, but I suppose we could just update any such tests to test that it matches generated then MRO formatted code.
There was a problem hiding this comment.
I agree I had to bend over backwards to get this formatting right and it's much simpler if I don't need to
There was a problem hiding this comment.
Fair enough! Should we simply auto-generate an mro that has the correct syntax and let the user pipe it through mro format downstream?
There was a problem hiding this comment.
Sounds like both of you agree with that strategy so I will revert it to the unformatted version.
We will need to update rust-martian github action tests to perform mro format before comparison when we bump martian-rust in that repo
There was a problem hiding this comment.
I think it's a good idea to have it at least produce semi-formatted output, if for no other reason than to makes it easier to debug if mro format chokes on it due to a syntax error. But I don't think you should go to extreme lengths to replicate the formatting exactly, especially since I reserve the right to change things about the formatting in the future.
There was a problem hiding this comment.
I think I'll just leave it as-is. It's nice that the output matches MRF closely. and if we have small differences in certain MROs since they aren't exactly the same we will be sure to run MRF afterward
Co-authored-by: Sreenath Krishnan <sreenath.krishnan@10xgenomics.com>
Co-authored-by: Sreenath Krishnan <sreenath.krishnan@10xgenomics.com>
…//github.com/martian-lang/martian-rust into pe/implement-struct-descriptions-and-renames
