-
Notifications
You must be signed in to change notification settings - Fork 162
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
Optional registry #486
Optional registry #486
Conversation
This is looking good! Just a couple more lines to cover. Lint and docs don't like the docstrings, so I'll suggest improvements to fix those jobs. |
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.
These changes should get lint and docs to pass.
Co-authored-by: Steve Piercy <[email protected]>
yes, I'll have a look. I switched computer three times in the last week, so need to take a moment to get a proper environment up and running. I can't see why/how 51b5418 could have changed anything for the functional tests though |
@tdamsma I approve your work. Would you please review my docstring improvements and documentation additions in the last commit? I think I got it right, but it helps to have a second pair of eyes. Thank you! |
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 improvements, please go ahead and finish it. I put some minor comments, feel free to ignore them
I think we're done. w00t! Thank you @tdamsma! |
Implements changes suggested in #485.
I already put it up here for review, but did not properly document the changes yet.