-
Notifications
You must be signed in to change notification settings - Fork 38
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 support for GitHub repository contents #167
Add support for GitHub repository contents #167
Conversation
This looks great! @Dhaulagiri feel free to roll this out on a schedule that is convenient for your app, if this is good to go. |
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.
Looks great! Left a few cleanup comments.
|
||
test('retrieving github repository contents', function(assert) { | ||
assert.expect(4); | ||
server.logging = true; |
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.
this can be removed
file: 'app.json' | ||
}).then((content) => { | ||
assert.githubRepositoryContentsOk(content); | ||
assert.equal(store.peekAll('githubRepositoryContents').get('length'), 1); |
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.
can you add a 3rd parameter for this assert loads 1 repository contents
?
@@ -0,0 +1,12 @@ | |||
import GitHubAdapter from './github'; |
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 prefer to explicit on these imports so that any file moves don't surprise us. Could you update this and all other new imports to have ember-data-github/<thing>/<name>
in them
thanks @elwayman02 & @Dhaulagiri ! |
7334db5
to
fdd0f50
Compare
@Dhaulagiri done and done |
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!!
We found a candidate model on the Heroku Dashboard for something that could live inside this addon