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 Tool\CrossIndex class #853

Merged
merged 1 commit into from
May 29, 2015
Merged

Added Tool\CrossIndex class #853

merged 1 commit into from
May 29, 2015

Conversation

webdevsHub
Copy link
Contributor

Implementation of #830, #473, #829, #836.

In unreleased https://github.com/ruflin/Elastica/blob/reindex-refactoring-to-copy/lib/Elastica/Util.php#L199 branch reindex/copy was added to Elastica\Util class. It does not belong there in my opinion if you look at the other functions in that class. So I created a new Tool namespace.

That namespace can be used by #840 as well (or any other future tools).

@ruflin
Copy link
Owner

ruflin commented May 20, 2015

@webdevsHub Can you briefly elaborate on what belongs into Util and what into the Tool namespace? We should document this somewhere so we can use it for future features to decide where they belong. The part I don't like too much is that we have a class with only static functions. It feels a little bit "wrong" :-)

@webdevsHub
Copy link
Contributor Author

Naming is inspired by Doctrine. Utility is a collection of things you need for internal Elastica coding and Tool for complex functionality for lib users (in Doctrine it is Pagination and the console commands). Do you have any other ideas for a good naming?

I used static like in Util because there is no state and therefore no need to create an Instance

$crossIndex = new CrossIndex();
$crossIndex->reindex(/*...*/);

=> more code overhead as

CrossIndex::reindex(/*...*/);

@ruflin
Copy link
Owner

ruflin commented May 29, 2015

I added a note here and merged your pull request. 288a1ec

ruflin added a commit that referenced this pull request May 29, 2015
Added Tool\CrossIndex class
@ruflin ruflin merged commit 37a9893 into ruflin:master May 29, 2015
@webdevsHub
Copy link
Contributor Author

stooop :) changelog missing

@webdevsHub
Copy link
Contributor Author

I am going to add it immediately

@ruflin
Copy link
Owner

ruflin commented May 29, 2015

Missed that one. Sorry.

webdevsHub added a commit to webdevsHub/Elastica that referenced this pull request May 29, 2015
ruflin added a commit that referenced this pull request May 29, 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.

2 participants