-
Notifications
You must be signed in to change notification settings - Fork 2.1k
mmv1: add FieldType() helper for documentation generation
#15724
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
mmv1: add FieldType() helper for documentation generation
#15724
Conversation
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
mmv1/api/type.go
Outdated
| return strings.TrimSpace(strings.TrimRight(t.Description, "\n")) | ||
| } | ||
|
|
||
| func (t *Type) FieldType() string { |
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 should return a list of strings, which is joined in the template with the join helper, as described in #15385 (review).
Please also add unit tests.
|
removing service labels / review request since is a provider-wide change that just makes the docs a little easier to maintain. |
|
The reason this should return a slice of strings is because we don't want to lock ourselves in to it being a string here; it limits the utility of the function. Additionally, it breaks the separation of concerns: the template is responsible for formatting the data we give it; handling that formatting in a helper method takes that responsibility away from the template (and makes it harder to understand what the final output of the template will be.) There may be other helper methods that format & return strings to be directly inserted into the template, but that's generally a bad practice and we should remove them over time. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
melinath
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.
This is great, thanks for the thorough unit tests! The docs output does diverge slightly in that it adds "write-only" to things that are in the write-only section at the moment, but that's fine. It doesn't lose information, and your other PR is about to remove that section anyway!
6a6634d
implements a helper function first mentioned here:
mmv1/docs: movewrite-onlyarguments intoargumentssection #15385 (review)after initial work on improving the documentation generation.
This moves the logic found within
property_documentation.html.markdown.tmplto its own helper method to handle the type of field when generating documentation (i.e output, write-only, required)Validation on this is ensuring that we get no diffs when generating the docs
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.