Skip to content
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

Generate READMEs for all components using a script #484

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

RobbeSneyders
Copy link
Member

This PR adds a script to generate a component README from its fondant_component.yaml.

As a next step, I'd like to add these to the documentation.

Copy link
Contributor

@PhilippeMoussalli PhilippeMoussalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :) This will save us a lot of time

Few questions:

  1. The defaults are not show in the argument table. Is it worth adding them (perhaps explicitly mention that those are optional) and then not showing them in the Usage section? I feel like this will show better that default arguments don't need to be passed but might be less clear
  2. We mention now testing but we don't have tests for every component. Can we dynamically create this segment only if we have a test for the component?
  3. How is this script run? is it going to be part of CI/CD or should we do it manually every time we create a component?

components/caption_images/README.md Outdated Show resolved Hide resolved
components/download_images/README.md Outdated Show resolved Hide resolved
components/download_images/README.md Outdated Show resolved Hide resolved
Copy link
Member Author

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The defaults are not show in the argument table. Is it worth adding them (perhaps explicitly mention that those are optional) and then not showing them in the Usage section? I feel like this will show better that default arguments don't need to be passed but might be less clear

I added the defaults to the table, and commented them in the Usage section. I think this makes it clear that they are not needed while still providing an example. If no default is available, I now generate the default value of the type as an example.

  1. We mention now testing but we don't have tests for every component. Can we dynamically create this segment only if we have a test for the component?

Done

  1. How is this script run? is it going to be part of CI/CD or should we do it manually every time we create a component?

I added it to pre-commit. I think that's the most logical place since it makes changes to the code.

components/download_images/README.md Outdated Show resolved Hide resolved
components/download_images/README.md Outdated Show resolved Hide resolved
components/filter_comments/README.md Show resolved Hide resolved
components/caption_images/README.md Outdated Show resolved Hide resolved
@RobbeSneyders RobbeSneyders force-pushed the feature/component_readmes branch from 484009f to c566b3b Compare October 4, 2023 19:43
Copy link
Contributor

@mrchtr mrchtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RobbeSneyders! I'm already a big fan of it.

Copy link
Contributor

@PhilippeMoussalli PhilippeMoussalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Robbe!

@RobbeSneyders RobbeSneyders merged commit 096b5dd into main Oct 5, 2023
5 checks passed
@RobbeSneyders RobbeSneyders deleted the feature/component_readmes branch October 5, 2023 08:01
Hakimovich99 pushed a commit that referenced this pull request Oct 16, 2023
This PR adds a script to generate a component README from its
`fondant_component.yaml`.

As a next step, I'd like to add these to the documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants