search

rapidsai / frigate

Frigate is a tool for automatically generating documentation for your Helm charts
https://frigate.readthedocs.io
Other
83 stars 25 forks source link

Add support for multiline json in tables #23

Open raul1991 opened 3 years ago

raul1991 commented 3 years ago

Pending items

GPUtester commented 3 years ago

Can one of the admins verify this patch?

raul1991 commented 3 years ago

Thanks for raising this.

A few thoughts:

  • Is there definitely no way to do this in pure markdown? I tried but failed miserably, read a few blogs, SO posts but everyone seems to be recommending the html way.
  • Does this break the other two templates (RST, HTML)? Not both, but I think a multiline json in the RST file breaks it. It would be great help if you can give some pointers on the RST problem because I have never worked with it.
  • Should we only use multiline code fences when there are multiple lines? I agree. I can put a conditional in jinja to make that work.

Please do share your feedback on the answers above so I continue.

jacobtomlinson commented 3 years ago

I tried but failed miserably, read a few blogs, SO posts but everyone seems to be recommending the html way.

Fair enough.

Not both, but I think a multiline json in the RST file breaks it. It would be great help if you can give some pointers on the RST problem because I have never worked with it.

I'm not sure if it is possible to do this in RST, but I'm afraid we will need to figure it out in order to get this merged.

I agree. I can put a conditional in jinja to make that work.

Great!

jacobtomlinson commented 3 years ago

It was recommended to me to check out list-tables in RST.

https://docutils.sourceforge.io/docs/ref/rst/directives.html#list-table

raul1991 commented 3 years ago

Regarding the HTML , enclosing the code in <pre> tag does the work.

raul1991 commented 3 years ago

Since the commits were dependent on each other, I just pushed in just one pull request. However, I have mentioned the issues it fixes in each of them.

raul1991 commented 3 years ago

Will try to fix the RST thing soon. I have to add the conditional check on the jinja as well for the multiline json. A question - Should I do this if "\n" in default to check if its multi line or not ?

What are your thoughts on it ?

raul1991 commented 3 years ago

Fixed the html thing as well. Now only the RST thing remains. :)

jacobtomlinson commented 3 years ago

Great!

You can check this out for a working RST example.

https://raw.githubusercontent.com/rapidsai/ucx-py/branch-0.17/docs/source/send-recv.rst

(Also for future reference please don't force push on GitHub, it helps to review to be able to see each commit. We will squash on merge anyway.)

raul1991 commented 3 years ago

So sorry for that. Ill take care of that moving forward. Just one thing to ask

I usually do this during pull requests

commit ---- comments fix ----- commit --amend

Since the last step is just an amend in the existing commit, do you think its reasonable to push N amends here than a single commit ? Just want to understand the intuition.

But I also see your point, squashing them at end gives the same effect.

raul1991 commented 3 years ago

One more thing, can I take up the tests for it in a different pull request ?

jacobtomlinson commented 3 years ago

It's best to not amend. Just keep adding new commits. That way GitHub shows me a view of what has changed since you last pushed. This makes it much easier to see how things are progressing. Otherwise I have to read the whole thing and try to figure out what you changed since the last time I looked at it.

Squashing at the end keeps the main commit history clean. But GitHub does this for us when we merge.

I would prefer tests to be included in this PR.

raul1991 commented 3 years ago

Sorry for stretching this for too long but couldn't get time to look at the rst format as of now. Can you or someone else take up the rst part of it ? Other things are already in place.