-
Notifications
You must be signed in to change notification settings - Fork 54
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
ENH
Adds ability to create table of contents
#305
ENH
Adds ability to create table of contents
#305
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.
Regarding not needing an extra utility function: you're right, we should keep it simple for now, this is sufficient.
Regarding your implementation, I think it won't work if you plan to use the anchors as they need to be lower-cased and may not contain white spaces (SO). Also not sure if we need links at all, as a user can't do anything with those, except when they're rendered, but as is, the rendered model card won't contain the TOC.
Moreover, four spaces looks excessive to me. I think 2 spaces should be enough to be rendered correctly.
Finally, could you please add some tests?
@BenjaminBossan This is ready for you to review again! |
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.
From my point of view, we're pretty close now. Just a few small changes.
Maybe @adrinjalali you could also take a quick look to see if this feature makes sense?
This reverts commit c2602f0.
@BenjaminBossan and @adrinjalali I've addressed all your comments other than including the Table of Contents in the markdown |
@adrinjalali I've addressed your comments! Sorry, it took a while! |
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 @lazarust
@adrinjalali and @BenjaminBossan This is ready for y'all to look at again! |
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 @lazarust , I'll let @BenjaminBossan have the final look.
Thanks a lot! |
Closes: #302