-
Notifications
You must be signed in to change notification settings - Fork 581
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
Use dataclasses for all objects returned by HfApi #1911 #1974
Conversation
@Wauplin I've updated the code for bloblfsinfo class. What do I need to make sure to commit the PR. |
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.
Hey @Ahmedniz1 thanks for starting on this PR. I've left 2 small comments on it. Apart from that, you'll need to:
- Make sure all instantiations of
BlobLfsInfo
are now passing a dictionary as input. For example, this line must be replaced bylfs = BlobLfsInfo(lfs)
- Format your code to comply with the codebase rules. To do so, install the dev dependencies (
pip install -e ".[dev]"
) and run the formatter (make style
). Then you can check everything's good (make quality
) and finally commit the changes done to the code (most likely additional whitespace + newlines). - Finally it is generally a good practice to add a short description to the PR (here) and especially mention that it's solving issue Use
dataclasses
for all objects returned byHfApi
#1911. This way we have a connection between the issue and the related PR.
That should be it! Thanks for your contribution! :)
@Wauplin I've made the required changes, I hope it resolves the conversion of bloblfsinfo class. If there is any issue in it, please do let me know as I'm new to open source. |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
Hi @Ahmedniz1, thanks for making the changes! The CI failed tests made me realize that the current solution that I've suggested to make is not satisfying in term of backward compatibility. I'm suggesting you some new changes that should make it cleaner and more robust for downstream apps. I'm sorry about this change of instructions, the fault is on my end. Better to fix it now rather than when it's fixed though :)
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 for making the changes @Ahmedniz1! Ending up with a small and clean PR in the end which is nice :) Let's wait for the CI to complete and then we should be fine to merge this! 🎉
It's green! Merging it now. Thanks again :) |
@Ahmedniz1 yes you can! Thanks for the help. I think NouamaneELGueddarii is working on the |
No description provided.