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

Proposal to add custom notation type #87

Merged
merged 1 commit into from
May 10, 2022

Conversation

lucernae
Copy link
Contributor

@lucernae lucernae commented Feb 2, 2021

As a reference to the issue #86,

I attached my PR here for review and consideration.

@lucernae
Copy link
Contributor Author

lucernae commented Feb 2, 2021

I attached a sample generated README.md It has to be viewed from my branch so Github can render it as usual.

@lucernae
Copy link
Contributor Author

lucernae commented Feb 2, 2021

Seems I failed the original unittest. I will try to understand the case by generating from the yaml

@lucernae
Copy link
Contributor Author

Hi, do you have some comments on this?

@norwoodj
Copy link
Owner

Hello @lucernae I'm sorry, I haven't had time to look at this project much this year. I will try to take a thorough look soon

@lucernae lucernae force-pushed the notation-type branch 2 times, most recently from e1cd68b to 76d8026 Compare July 2, 2021 17:10
@seboudry
Copy link

seboudry commented Oct 11, 2021

Hi @lucernae !
Great work on this one!
I'm also interrested for these cutomizations on values.

Really like the K8S types with link to documentation 👍

I've got a several comments :

Why add use tpl/ in -- (tpl/xxx) for type?
Does @notationType work alone or in combination with the (tpl/xxx) type?
In generated readme, field types contains tpl but it's only useful for rendering.

Why add -- in following bloc?

# @notationType -- nginx

Is this mandatory for parsing?
If not, this only add unnecessary caracters.

Does these customizations also work in markown tables?

@lucernae
Copy link
Contributor Author

Hi @seboudry

As a disclaimer, I submit this PR as a review for a proposal described here: #86 .
So this is mainly ideas I throw around to request for more comments if anybody is interested on supporting this kind of things.

As for your question:

Why add use tpl/ in -- (tpl/xxx) for type?

This is for semantic purposes only. If the notation type is (tpl/array), that means it is a Go tpl string that will render to an array.
This is useful for the renderer if they decided to add custom styling. The renderer can understand that the value is a Go template string (they can opt to render it as Go template), and that the computed values of the template is a YAML array.
Lots of community Helm charts allows YAML key values with a Go template string, but it's not clear what type the template string will render and it can't be inferred. So this will help provide context for the renderer. For example, the renderer can provide links to the type description given such information

Does @notationType work alone or in combination with the (tpl/xxx) type?
In generated readme, field types contains tpl but it's only useful for rendering.

The @notationType only provides notationType field in the renderer. So your renderer can use this information. That means the tpl/xxx can be in any format/convention your renderer can use. In this example, I just made that tpl/xxx means it is a Go template string that will render into an xxx type. The notationType only gives you framework to implement a custom renderer for a type, or link value to the type definition. You need to implement the renderer yourself.

Why add -- in following bloc?

No particular reason. I'm just trying to follow the same convention with existing helm-docs annotations. In the project readme, the author uses @default -- annotation value, so I am trying to use the same mental model.

@seboudry
Copy link

OK, thanks for your explanations.

I hadn't seen on the first time the connection between the tpl/xxx type and Go template.
That seems a good improvement in readability and comprehension when reading the doc.

👏

@norwoodj some free time to take a look at this one? 😉

@norwoodj
Copy link
Owner

norwoodj commented Apr 3, 2022

Apologies for being so slow to look at this. I am going to try to take a look at this soon, it is just hard to find the time between life and my real job.

@norwoodj
Copy link
Owner

I took a look at this today. I think this is maybe something worth incorporating, but I'm having trouble understanding what the intended use case is. I think it would help me both understand better and save me some future work if you documented the new features you're adding in the project README.

Beyond that, I have a couple of requests:

  • Please rebase and fix merge conflicts
  • Squash your changes into a single commit.

Thanks again for all the work you've put into this, I do want to merge these changes, I'm just not yet sure I understand how it would be used.

@lucernae
Copy link
Contributor Author

lucernae commented May 5, 2022

Hi @norwoodj , thanks for looking at it. Sorry for the late reply, I've been on holiday. I'm going check the recent conflicts first to make sure it is up to date with master, then I will ping you again when it is ready for review.

Add test files:
- Test custom overrides
- Test hyperlinks
- Test hyperlinks to value

Improvements:
- Add description about the hyperlinks
- Add more sample in the README.md
- Add support for multiline description sections
- Modify comment continuation regex
@lucernae
Copy link
Contributor Author

lucernae commented May 6, 2022

It doesn't seem the push triggers the CI. I'm going to close this PR and reopen, so that GH actions will be triggered on reopen.

@lucernae lucernae closed this May 6, 2022
@lucernae lucernae reopened this May 6, 2022
@lucernae
Copy link
Contributor Author

lucernae commented May 6, 2022

Ok, these are the changes:

  • Please rebase and fix merge conflicts
  • Squash your changes into a single commit.
  • Tested unittest in pkg/document
  • Showcase the usage of @notationType metadata and custom rendering in example-charts/custom-value-notation-type/README.md. I think this is the best place to see what kind of improvements I want to bring.
  • Added sections for custom rendering in README.md
  • Added built-int template to render HTML tables in README.md

@norwoodj norwoodj merged commit ef73e2e into norwoodj:master May 10, 2022
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