Skip to content
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

Additional objects #2652

Merged
merged 1 commit into from
Apr 28, 2014
Merged

Additional objects #2652

merged 1 commit into from
Apr 28, 2014

Conversation

sebgie
Copy link
Contributor

@sebgie sebgie commented Apr 25, 2014

closes #2620

  • moved aspect -> filters
  • updated tests
  • fixed inconsistency in pagination object
  • changed filter object to filters: {tags:[{id: 1, ...}]}

@@ -398,11 +399,13 @@ Post = ghostBookshelf.Model.extend({
pagination.prev = pagination.page - 1;
}
}

// console.log(tagInstance);

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@hswolff
Copy link
Contributor

hswolff commented Apr 25, 2014

I didn't wrap the filters in an array as I think that this should be done when we implement the functionality.

I think it'd be good to get the filter returned object consistent and create it as an array from the start.

We do know that it will be the case that we'll support multiple filters as querying for a certain tag with a given author is something that we'll want. If it was more hazy about what this filter param would support then I'd agree we should wait, but this is something that we know is coming down the pipeline.

@sebgie
Copy link
Contributor Author

sebgie commented Apr 25, 2014

@hswolff I didn't want to anticipate any future implementation. I could update the object as follows:

filters: [{
    tags: [{
        id: 1,
        ....
    }]
}]

It will then use the first element of the first tags object for now and if multiple resources are needed we can update it accordingly?

@ErisDS
Copy link
Member

ErisDS commented Apr 25, 2014

👍

@hswolff
Copy link
Contributor

hswolff commented Apr 25, 2014

I'm imagining an API that would request posts?tags=foo,bar&author=2 which I'd hope to get back an API of:

filters: {
    tags: [{
        id: foo,
        ....
    }, {
        id: bar
    }],
    authors: [{
        id: 2
    }]
}

Then again I don't want to go down the road of premature optimization too much so take all I say with that grain.

I just do imagine this is a future we will have.

@sebgie
Copy link
Contributor Author

sebgie commented Apr 25, 2014

filters: [{},{}] or filter: {}?

That was the point where I decided to write:

I didn't wrap the filters in an array as I think that this should be done when we implement the functionality.

Without any code in place I think that your option using filter: {} seems better because otherwise we would end up with an array of different objects. The array version would imo be closer to JSON API if your point of view is that every object (tag/author/...) represents a filter?

@ErisDS
Copy link
Member

ErisDS commented Apr 25, 2014

I am hoping for that future to be a reality very, very soon 😸

@ErisDS
Copy link
Member

ErisDS commented Apr 25, 2014

I think @hswolff's structure makes sense?

@hswolff
Copy link
Contributor

hswolff commented Apr 25, 2014

That was the point where I decided to write:

Aha, I mis-read then, my mistake, apologies. Carry on then. :B

@sebgie
Copy link
Contributor Author

sebgie commented Apr 25, 2014

@hswolff no, I'm happy if we get to a decision :shipit:.

If no one has an objection to the following format we should use it. I took @hswolff's structure and renamed filters to filter.

filter: {
    tags: [{
        id: foo,
        ....
    }, {
        id: bar
    }],
    authors: [{
        id: 2
    }]
}

@ErisDS
Copy link
Member

ErisDS commented Apr 25, 2014

👍

1 similar comment
@hswolff
Copy link
Contributor

hswolff commented Apr 25, 2014

👍

closes TryGhost#2620
- moved aspect -> filters
- updated tests
- fixed inconsistency in pagination object
@sebgie
Copy link
Contributor Author

sebgie commented Apr 27, 2014

Updated! I think the build needs a restart as it worked on my repo (https://travis-ci.org/sebgie/Ghost/builds/23877352).

ErisDS added a commit that referenced this pull request Apr 28, 2014
@ErisDS ErisDS merged commit c347d3f into TryGhost:master Apr 28, 2014
ErisDS added a commit that referenced this pull request Apr 28, 2014
fixes the build

- PR #2238 added an extra reference to aspect which wasn't fixed by #2652, this resolves that
@sebgie sebgie deleted the issue#2620 branch May 6, 2014 09:33
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.

Post API: Returning additional objects
3 participants