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

Nested properties #27

Merged
merged 4 commits into from
Mar 7, 2016
Merged

Nested properties #27

merged 4 commits into from
Mar 7, 2016

Conversation

noirbizarre
Copy link
Contributor

This pull request add support for searching and sorting on nested properties using dot-notation when declaring fields (ie. nested.property).

I tried updating the documentation but I'm not sure on where to put this.

This pull request fix #26.

@noirbizarre
Copy link
Contributor Author

Hi !

Is there any chance this can be reviewed and if it fit, merged ?

If not, what can I do to make it mergeable ?

@noirbizarre
Copy link
Contributor Author

Hi !
I just saw that you merged some pull-request but not this one and not any comment.

Is there any reason not to merge this one ?
If there is, what can I do/change to make it mergeable ?

@brianreavis
Copy link
Owner

This may make no visible change to the API, but it does come with a performance hit. If this is to be merged, it needs to either (1) be a completely different code path that's enabled via an option like nesting:true or (2) compile the accessor.

@noirbizarre
Copy link
Contributor Author

Thanks for the feedback. I will adjust the PR.

I fear that the completely different path introduce too much complexity and maintainability drop.

I'm not sure about the second option meaning (compile the accessor), what do you mean by compile ? Make the getattr a native function ?

I just noted the benchmark task. I will try to find a solution with less performance hit.
What value do you find acceptable ?

@noirbizarre
Copy link
Contributor Author

OK, I just:

  • made the nesting optional with the nesting search option
  • documented the parameter and the potential side-effect
  • rebased on master

<tr>
<td valign="top">"nesting"</td>
<td valign="top">boolean</td>
<td valign="top">If <code>true</code>, nested fields will be available for search and sort using dot-notation to reference them.<br>ex:<code>nested.property</code><br><em>Warning: can reduce performances</em></td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nitpick: "performances" should be "performance"

Nevermind, will just fix this up in master :)

@holic
Copy link
Collaborator

holic commented Mar 7, 2016

Looks great. Thanks for the PR!

holic added a commit that referenced this pull request Mar 7, 2016
@holic holic merged commit 2b27ba0 into brianreavis:master Mar 7, 2016
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.

Allow to sort on nested properties
3 participants