Skip to content

ENH Adds ability to create table of contents#305

Merged
BenjaminBossan merged 14 commits intoskops-dev:mainfrom
lazarust:ENH-add-card-table-of-contents
Mar 13, 2023
Merged

ENH Adds ability to create table of contents#305
BenjaminBossan merged 14 commits intoskops-dev:mainfrom
lazarust:ENH-add-card-table-of-contents

Conversation

@lazarust
Copy link
Copy Markdown
Contributor

Closes: #302

Copy link
Copy Markdown
Collaborator

@BenjaminBossan BenjaminBossan left a 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?

@lazarust lazarust marked this pull request as ready for review March 1, 2023 01:20
@lazarust lazarust requested a review from BenjaminBossan March 1, 2023 01:23
@lazarust
Copy link
Copy Markdown
Contributor Author

lazarust commented Mar 1, 2023

@BenjaminBossan This is ready for you to review again!

Copy link
Copy Markdown
Collaborator

@BenjaminBossan BenjaminBossan left a 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?

Comment thread skops/card/_model_card.py Outdated
Comment thread skops/card/_model_card.py
Comment thread skops/card/tests/test_card.py Outdated
Comment thread skops/card/_model_card.py
Comment thread docs/changes.rst Outdated
Comment thread skops/card/_model_card.py
Comment thread skops/card/_model_card.py Outdated
@lazarust
Copy link
Copy Markdown
Contributor Author

lazarust commented Mar 2, 2023

@BenjaminBossan and @adrinjalali I've addressed all your comments other than including the Table of Contents in the markdown

Comment thread skops/card/_model_card.py Outdated
Comment thread skops/card/_model_card.py Outdated
Comment thread skops/card/_model_card.py
@lazarust
Copy link
Copy Markdown
Contributor Author

lazarust commented Mar 7, 2023

@adrinjalali I've addressed your comments! Sorry, it took a while!

@lazarust lazarust requested a review from BenjaminBossan March 7, 2023 01:01
Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Thanks @lazarust

Comment thread skops/card/_model_card.py
Comment thread skops/card/_model_card.py Outdated
Comment thread skops/card/_model_card.py
@lazarust lazarust requested review from BenjaminBossan and adrinjalali and removed request for BenjaminBossan and adrinjalali March 11, 2023 00:18
@lazarust
Copy link
Copy Markdown
Contributor Author

@adrinjalali and @BenjaminBossan This is ready for y'all to look at again!

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a 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.

@BenjaminBossan
Copy link
Copy Markdown
Collaborator

Thanks a lot!

@BenjaminBossan BenjaminBossan merged commit e74dbf3 into skops-dev:main Mar 13, 2023
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.

Add a method to model cards to generate an overview of sections

3 participants