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

ENH Adds ability to create table of contents #305

Merged
merged 14 commits into from
Mar 13, 2023

Conversation

lazarust
Copy link
Contributor

Closes: #302

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

lazarust commented Mar 1, 2023

@BenjaminBossan This is ready for you to review again!

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

skops/card/_model_card.py Outdated Show resolved Hide resolved
skops/card/_model_card.py Show resolved Hide resolved
skops/card/tests/test_card.py Outdated Show resolved Hide resolved
skops/card/_model_card.py Show resolved Hide resolved
docs/changes.rst Outdated Show resolved Hide resolved
skops/card/_model_card.py Show resolved Hide resolved
skops/card/_model_card.py Outdated Show resolved Hide resolved
@lazarust
Copy link
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

skops/card/_model_card.py Outdated Show resolved Hide resolved
skops/card/_model_card.py Outdated Show resolved Hide resolved
skops/card/_model_card.py Show resolved Hide resolved
@lazarust
Copy link
Contributor Author

lazarust commented Mar 7, 2023

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

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

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

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

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