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

Implemented index template support #905

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

dbalabka
Copy link
Contributor

@dbalabka dbalabka commented Aug 5, 2015

*
* @var string Index pattern
*/
protected $name = '';
Copy link
Owner

Choose a reason for hiding this comment

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

I know that symfony has the naming standard without the underline, but as the Elastica project everywhere use $_* for protected variables, please adjust it.

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 just made it like that because in contributing notes is mentioned that code must be in PSR-2 code style, but exactly PSR-2 says that:

Property names SHOULD NOT be prefixed with a single underscore to indicate protected or private visibility.
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#42-properties

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, we take the SHOULD part here ;-) I fixed it and pushed it to master. Thanks for the contribution.

@ruflin
Copy link
Owner

ruflin commented Aug 6, 2015

In general this looks good to me. It would be good to have 1-2 more functional tests. This would also discover in case the API changes in the future.

@ruflin
Copy link
Owner

ruflin commented Aug 6, 2015

@torinaki I'm happy to merge the pull request if you adjust the coding styles above so FriendsOfSymfony/FOSElasticaBundle#916 can be pushed forward.

@mablae
Copy link

mablae commented Aug 7, 2015

Another awsome feature! I will use it. Thanks.

@ruflin ruflin merged commit d87495e into ruflin:master Aug 10, 2015
@ruflin
Copy link
Owner

ruflin commented Aug 10, 2015

@torinaki I made the adjustments here and merged it: f64e8ae It would be nice if you could add some more integration tests later.

@dbalabka
Copy link
Contributor Author

@ruflin thank you for quick response. I'll prepare more functional tests with another PR soon

@ruflin
Copy link
Owner

ruflin commented Aug 10, 2015

👍

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.

3 participants