Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Add support for multiline json in tables #23

Open
wants to merge 3 commits into
base: branch-0.3
Choose a base branch
from

Conversation

raul1991
Copy link
Contributor

@raul1991 raul1991 commented Nov 7, 2020

  • Since MD syntax does not really support it so I've created
    table using html table syntax and it seems to render perfectly.

Pending items

  • rst and html templates.

@GPUtester
Copy link

Can one of the admins verify this patch?

@raul1991 raul1991 force-pushed the branch-0.3 branch 2 times, most recently from b663b5a to c279360 Compare November 7, 2020 17:33
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for raising this.

A few thoughts:

  • Is there definitely no way to do this in pure markdown?
  • Does this break the other two templates (RST, HTML)?
  • Should we only use multiline code fences when there are multiple lines?

@raul1991
Copy link
Contributor Author

raul1991 commented Nov 9, 2020

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
Copy link
Member

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
Copy link
Member

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

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

@raul1991
Copy link
Contributor Author

raul1991 commented Nov 9, 2020

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

@raul1991
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

- Since MD syntax does not really support it so I've created
table using html table syntax and it seems to render perfectly.

Pending items
--------------

- rst and html templates.

Fixes rapidsai#17 rapidsai#24
If a user packs the chart properly (including its dependent charts)
so he would only like frigate to just go through them and just dump
the values from them that way it already does.
The only thing would be to NOT run the repo update and
the dep update commands.
One thing to note here is that the above commands only slow down
the whole recursion drastically (I had to wait for 20 mins).

With this improvement a flag called --no-update has been introduced
which would not invoke the the aforementioned commands
during the traversal.

Fixes rapidsai#26
@jacobtomlinson
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

raul1991 commented Nov 10, 2020

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

@jacobtomlinson
Copy link
Member

jacobtomlinson commented Nov 11, 2020

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.

- Added test for pre-packaged chart
- Corrected the assertion for a test
@raul1991
Copy link
Contributor Author

raul1991 commented Feb 6, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants