-
Notifications
You must be signed in to change notification settings - Fork 112
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 source description #40
Add source description #40
Conversation
Hi @djbelknapdbs — thanks so much for this PR! The tests are failing because of an unrelated issue (one that we fixed in #39). To get past this issue, try rebasing (or merging in) the latest changes Once that's fixed, I think you'll also find that the existing integration tests start failing — we have logic that is not expecting these description keys Finally, once we have passing code, I think we'd want to implement this as an argument, e.g. So there's a fair bit to do:
Let me know if you're up for this, or if all that feels like too much work (a totally reasonable response!) |
Hi @clrcrl - thanks for the review. I'm just getting going on contributing and the testing part in particular. I think I got these issues squared away. I hijacked an existing test since it referred to all args, but if you'd rather it's a new test I can do that too. |
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.
@djbelknapdbs This is great! Sorry for the delay, thanks for responding to Claire's feedback, all in all this LGTM.
Add source description
Description & motivation
Add the placeholder for
description
to sources requested in #17 and #28 .Checklist