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

Add options to find query #14

Merged
merged 1 commit into from
Feb 2, 2016
Merged

Add options to find query #14

merged 1 commit into from
Feb 2, 2016

Conversation

lionelrudaz
Copy link
Contributor

No description provided.

where, order,
limit: filters.$limit,
offset: filters.$skip,
attributes: filters.$select || null
};

}, params.sequelize);
Copy link
Member

Choose a reason for hiding this comment

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

👍 can we put this under $sequelize instead? That way it follows a similar pattern to the other special query params?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's ok. It's part of params not params.query (where we use that pattern to somewhat avoid property name clashes).

Copy link
Member

Choose a reason for hiding this comment

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

ah right. My bad. ok let's :shipit:

@ekryski
Copy link
Member

ekryski commented Feb 1, 2016

I'm cool with this. We should adds some docs around this as well. @feathersjs/core-team where do you want to put them? In the docs or leave something specific like this in the README? I'm inclined to put it in the docs so that it all stays together.

Refs #12.

@marshallswain
Copy link
Member

Definitely in the docs repo.

@ekryski
Copy link
Member

ekryski commented Feb 1, 2016

Ok then I'll create a corresponding issue in the feathers-docs repo.

@lionelrudaz
Copy link
Contributor Author

Thanks guys. Let me know if I can be of any help.

daffl added a commit that referenced this pull request Feb 2, 2016
@daffl daffl merged commit 4aec595 into feathersjs-ecosystem:master Feb 2, 2016
@daffl
Copy link
Member

daffl commented Feb 2, 2016

Thank you for contributing! I just published a patch release with your fix.

@lionelrudaz
Copy link
Contributor Author

You're far too kind. Thanks for your help.

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.

4 participants