-
Notifications
You must be signed in to change notification settings - Fork 571
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
Add settings for creating and duplicating spaces #1625
Add settings for creating and duplicating spaces #1625
Conversation
I tried to come up with tests for duplicating spaces. I'll probably leave this with you @Wauplin if you don't mind. You probably have the best idea of how to mock this... |
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.
That's a very good quality PR, thanks for the work @martinbrose ! I have added comments about renaming/slightly rephrasing but that's it. About the duplicate_space
tests, I think you can use the exact same approach as the mocked create_repo
tests. With mock tests you cannot be rate-limited since http requests are not made to the Hub. I'm fine with adding them myself if you prefer.
Also to complete this PR, would you mind updating the Manage your Space guide in the docs? You can update them in the docs/
folder. There is a section in a guide "Bonus: request hardware when creating the Space!". I would update it to show how to set hardware + sleep time + secrets + variables at the same time at repo creation. There is also a guide for duplicating Spaces that would benefit from a mention of the new arguments you've added. I would keep the existing minimal example and a another complete example just below and explain in a few words why it's useful. Let me know what you think.
Thanks in advance!
The documentation is not available anymore as the PR was closed or merged. |
Thanks for the review and the kind recognition. I addressed all the code review points and added the documentation. |
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.
Nice improvement in the docs! Waiting for the final tests for duplicate_space before merging :)
Hi @Wauplin, I think I was overthinking the test case... we are only testing if the API call is correct. This is now addressed in the additional test case. |
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.
Great job @martinbrose! Thanks for the last test, it's perfect like this. The PR is fine to merge now :) 🔥
References #1457
Key Features