Skip to content

add fetchInitialRevisions #50

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

Merged
merged 1 commit into from
Feb 1, 2016
Merged

Conversation

ghedamat
Copy link
Contributor

@ghedamat ghedamat force-pushed the add-fetch-initial-revisions branch from fcd01fb to 00f725a Compare January 24, 2016 12:08
@ghedamat
Copy link
Contributor Author

@lukemelia initially I merged revisionData from fetchInitialRevisions manually but this https://github.com/ember-cli/ember-cli-deploy/blob/master/lib/models/pipeline.js#L168-L172 should do that for us right?

@lukemelia
Copy link
Contributor

@ghedamat While writing 0.6.x docs, I rethought this a little (see results at http://ember-cli.com/ember-cli-deploy/docs/v0.6.x/pipeline-hooks/)

fetchInitialRevisions -->  returns an object (or a promise resolving to
                           one) that has an `initialRevisions` property
                           whose value is an array of revision objects.[1]
                           i.e. `{ initialRevisions: [...] }`

and

            \- fetchRevisions  returns an object (or a promise resolving
             \                 to one) that has a `revisions` property
              \                whose value is an array of revision
               \               objects.[1] i.e. `{ revisions: [...] }`

@lukemelia
Copy link
Contributor

This would indeed be a breaking change. We need to think about backwards compatibility and/or messaging.

@ghedamat
Copy link
Contributor Author

@lukemelia I tend to agree, if we keep the properties separate we don't need to break any existing plugins and we can "reserve" the property names in the docs as you did

I'll update the PR and do the other ones later this week

@ghedamat ghedamat force-pushed the add-fetch-initial-revisions branch from 00f725a to f8ebacc Compare January 27, 2016 11:58
@ghedamat
Copy link
Contributor Author

@lukemelia should be good now and non breaking

@ghedamat ghedamat changed the title [BREAKING] add fetchInitialRevisions and move to revisionData key add fetchInitialRevisions and move to revisionData key Jan 27, 2016
@ghedamat ghedamat changed the title add fetchInitialRevisions and move to revisionData key add fetchInitialRevisions Jan 27, 2016
@ghedamat ghedamat force-pushed the add-fetch-initial-revisions branch from f8ebacc to 3952614 Compare January 27, 2016 17:37
describe('fetchRevisions hook', function() {
it('fills the revisions variable on context', function() {
describe('fetchInitialRevisions hook', function() {
it('fills the revisionsData.initial variable on context', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test name is no longer correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, will fix soon

@ghedamat ghedamat force-pushed the add-fetch-initial-revisions branch from 3952614 to db146a8 Compare February 1, 2016 14:22
ghedamat added a commit that referenced this pull request Feb 1, 2016
@ghedamat ghedamat merged commit 85bb524 into master Feb 1, 2016
@ghedamat ghedamat deleted the add-fetch-initial-revisions branch February 1, 2016 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants