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

Added support to parent_id query #1308

Merged
merged 1 commit into from
May 19, 2017
Merged

Conversation

giovannialbero1992
Copy link
Contributor

added ParentId Query by reference #1287

@giovannialbero1992 giovannialbero1992 changed the title Added support to parent_id query [#1287] Added support to parent_id query May 15, 2017
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I left a minor comment.

<?php
namespace Elastica\Query;

final class ParentId extends AbstractQuery
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you add final here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually apply final by default to classes that are not specifically designed to be extended.
This avoids breaking an eventual child class when refactoring this class.
If you want i can remove final from this class.

Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to remove it as we don't have it in any other files and I don't see a reason, why we should prevent someone extendings this class if we wants to (even though it is unlikely to happen).

Copy link
Contributor Author

@giovannialbero1992 giovannialbero1992 May 16, 2017

Choose a reason for hiding this comment

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

Ok, I've removed final

CHANGELOG.md Outdated
@@ -13,7 +13,7 @@ All notable changes to this project will be documented in this file based on the
### Added

- Parameter `filter_path` for response filtering (e.g. `$index->search($query, ['filter_path' => 'hits.hits._source'])`)

- Added `\Elastica\Query\ParentId` to avoid join with parent documents [#1287](https://github.com/ruflin/Elastica/issues/1287)
Copy link
Owner

Choose a reason for hiding this comment

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

Could also add here a newline before the next title?

@giovannialbero1992 giovannialbero1992 force-pushed the master branch 2 times, most recently from 0d3f11b to cfc1d5b Compare May 16, 2017 12:07
@giovannialbero1992 giovannialbero1992 force-pushed the master branch 2 times, most recently from 5c1ccc8 to 8643d83 Compare May 19, 2017 07:30
@ruflin
Copy link
Owner

ruflin commented May 19, 2017

@giovannialbero1992 Seems like you have changelog conflict :-(

@giovannialbero1992
Copy link
Contributor Author

giovannialbero1992 commented May 19, 2017

@ruflin Done! ;-)

@ruflin ruflin merged commit fa694e0 into ruflin:master May 19, 2017
@ruflin
Copy link
Owner

ruflin commented May 19, 2017

@giovannialbero1992 Thanks. merged.

mhernik pushed a commit to mhernik/Elastica that referenced this pull request Jul 24, 2017
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