-
Notifications
You must be signed in to change notification settings - Fork 29
Convert Attachment Object #360
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
base: develop
Are you sure you want to change the base?
Conversation
|
This is looking good to me; holding off on formally approving until #359 is resolved and the tests are pushed. |
Gonza10V
left a comment
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.
jakebeal
left a comment
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.
#359 may be merged, but this cannot be merged until it has tests added. @GeneCodeSavvy , can you please push the tests?
292e7fc to
5d1b062
Compare
|
tests are failing due to unrelated errors |
|
Re-running the failing tests gets past the rate-limits on the external services. |
jakebeal
left a comment
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.
I see test files, but not yet a test that actually makes use of them.
yep, i was waiting for some clarity about the tests. I have added the tests now |
c40da2a to
8bed8ce
Compare
|
@jakebeal the test are passing now |
This PR closes #244
I have not pushed the tests. They will fail due to #359
I will push the tests, once the mentioned PR is merged, fixing the backport issue