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

Expose RepoCard at top level + few qol improvements #1354

Merged
merged 9 commits into from
Feb 22, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Feb 21, 2023

Resolve #1348.
No need to add a SpaceCard class, it wouldn't have any additional features.

I took the opportunity to add a few QOL improvement to the CardData object (.get, __getitem__, __setitem__, __contains__, .pop). More of these could be added but honestly not worth the hassle (those 5 cover 99% of the use cases). Could have been simpler to just inherit from dict but this way we can explicitly add an "export" step when serializing the metadata.

Added some tests and completed a docstring.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 21, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@merveenoyan merveenoyan left a comment

Choose a reason for hiding this comment

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

looks good, I just skimmed through it :')

class SpaceCard:
"""Space card is an alias for [`RepoCard`].

At the moment, it does not implement any specific logic. `SpaceCard` is defined for
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it makes sense to have Space-specific attributes in RepoCard under this instead? (e.g. like the one that tells us about duplicate)

Copy link
Member

Choose a reason for hiding this comment

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

yes!!

Copy link
Contributor Author

@Wauplin Wauplin Feb 21, 2023

Choose a reason for hiding this comment

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

Defintely. Not useless to have a separate class in the end 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@nateraw nateraw left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks ❤️ this is super useful.

src/huggingface_hub/repocard.py Show resolved Hide resolved
src/huggingface_hub/repocard_data.py Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented Feb 22, 2023

Thanks for the reviews and feedback @merveenoyan @nateraw
I'm merging this PR :)

@Wauplin Wauplin merged commit 08ed62f into main Feb 22, 2023
@Wauplin Wauplin deleted the 1348-repo-card-at-top-level-and-few-qol-improvements branch February 22, 2023 13:55
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 SpaceCard class or make RepoCard as top-level + few tweak
5 participants