-
Notifications
You must be signed in to change notification settings - Fork 26
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
Update generic readme generation #737
Update generic readme generation #737
Conversation
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.
Thanks @PhilippeMoussalli! Some suggestions on how to make this even more clear.
{% if consumes %} | ||
**This component consumes:** | ||
|
||
{% for field_name, field in consumes.items() %} | ||
- {{ field.name }}: {{ field.type.value }} | ||
{% endfor %} | ||
{% else %} | ||
**This component consumes no data.** | ||
_**This component does not consume specific data.**_ | ||
{% endif %} | ||
|
||
{% if is_consumes_generic %} | ||
**This component consumes generic data** | ||
- <field_name>: <mapped_field_name> | ||
{% endif %} |
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 don't think "does not consume specific data", or "consumes generic data" is clear for users without deep knowledge about Fondant concepts. I think we can refactor this and update the messages to be more clear:
{% if consumes %}
**This component consumes:**
{% for field_name, field in consumes.items() %}
- {{ field.name }}: {{ field.type.value }}
{% endfor %}
{% elif is_consumes_generic %}
**This component can consume different data schemes**
See the usage example below on how to define a schema.
{% else %}
**This component does not consume data.**
{% endif %}
{% if produces %} | ||
**This component produces:** | ||
|
||
{% for field_name, field in produces.items() %} | ||
- {{ field.name }}: {{ field.type.value }} | ||
{% endfor %} | ||
{% else %} | ||
**This component produces no data.** | ||
_**This component does not produce specific data.**_ | ||
{% endif %} | ||
|
||
### Arguments | ||
{% if is_produces_generic %} | ||
**This component produces generic data** | ||
- <field_name>: <field_schema> | ||
{% endif %} |
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.
Same as proposal for consumes
above.
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.
Thanks @PhilippeMoussalli. Left a few comments.
### Arguments | ||
{% if is_produces_generic %} | ||
**This component produces generic data** | ||
- <field_name>: <field_schema> |
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 produce implementation can use both, a column mapping and a column/data type mapping. What you can use depends on the component implementation itself. Can we add a description for column name, data type mapping too?
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.
good call! added a description based on the docs
7d76bfb
to
c67ae37
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.
Some formatting issues now. You can see them if you look at the generated website.
{% if consumes %} | ||
**This component consumes:** | ||
|
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 newline is needed for formatting the list correctly.
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.
fixed
{% if produces %} | ||
**This component produces:** | ||
|
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 newline is needed for formatting the list correctly.
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.
fixed
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.
Thanks @PhilippeMoussalli. Lgtm
Fixes #729