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

ES-7: remove type #1666

Merged
merged 29 commits into from Oct 31, 2019

Conversation

@thePanz
Copy link
Collaborator

thePanz commented Oct 3, 2019

Todo:

  • Remove mapping's include_type_name (from #1654 )
  • Deprecate usage of Type object
  • copy setMapping() from Type to Index
  • copy getDocument() from Type to Index
  • copy deleteById() from TypetoIndex`
  • copy more methods from Type to Index
  • Cleanup Client methods requiring Type #1668
  • Remove Type class (<== needs clarification)
  • Cleanup Search->addType() #1669
  • Cleanup Search->addTypes() #1669
  • Cleanup Bulk class from Type #1670
  • Fix tests
@thePanz

This comment has been minimized.

Copy link
Collaborator Author

thePanz commented Oct 3, 2019

@ruflin the usage of include_type_name was too hard to remove while keeping a backward-compatibility. This PR is a first step to removing the Type completely, and moving its functionalities into the Index class.

@thePanz thePanz changed the title WIP: Es 7 remove type WIP: ES-7 remove type Oct 3, 2019
@renanbr

This comment has been minimized.

Copy link

renanbr commented Oct 7, 2019

Assuming everything is fine in src/ I changed some tests.
Let me know if it's in the good direction.

Changes I made, but not 100% convinced:

  • Elastica\Test\BulkTest::testSend() and Elastica\Test\BulkTest::testRawDocumentDataRequest()
    • orpahend document (has no index) is always tied to the type "_doc"

Wouldn't we deprecate (and auto-redirect when object is given) also these methods ?

  • Elastica\Bulk->setType() ==> Elastica\Bulk->setIndex()
  • Elastica\Search->addType() ==> Elastica\Search->addIndex()
  • Elastica\Search->addTypes() ==> Elastica\Search->addIndices()

Additional note:

  • Elastica\Client::deleteIds() and Elastica\Client::updateDocument() still require $type argument, is this intentional?
@thePanz

This comment has been minimized.

Copy link
Collaborator Author

thePanz commented Oct 7, 2019

@renanbr thank you for your findings, I updated the PR description

@renanbr

This comment has been minimized.

Copy link

renanbr commented Oct 10, 2019

@thePanz I've submitted some PRs:

  • [ES-7] Make Client class Type free #1668
  • [ES-7] Cleanup Search->addType() and Search->addTypes() #1669
  • [ES-7] Make Bulk classes Type free #1670 (Bulk weren't caught)

Let me know if they are in the good direction.

The missing tasks are:

  • Remore Type class
  • Fix tests*

* tests for these classes I've changed are green :)

@thePanz

This comment has been minimized.

Copy link
Collaborator Author

thePanz commented Oct 10, 2019

@renanbr I'll have a look asap.

I think that all your changes should be merged into this MR, to have all "remove Type class" changes in one single pull-request. wdyt?

update my bad, all your PR are targeting this branch :)

@thePanz thePanz force-pushed the es-7-remove-type branch 17 times, most recently from f40b9db to 3c51f0b Oct 10, 2019
lib/Elastica/Index.php Outdated Show resolved Hide resolved
lib/Elastica/Index.php Outdated Show resolved Hide resolved
lib/Elastica/Search.php Show resolved Hide resolved
@thePanz thePanz force-pushed the es-7-remove-type branch 2 times, most recently from 9ce86e9 to 7aa1778 Oct 11, 2019
@renanbr

This comment has been minimized.

Copy link

renanbr commented Oct 11, 2019

more test fixes #1674 (targeting this branch, as usual)

@renanbr

This comment has been minimized.

Copy link

renanbr commented Oct 14, 2019

Tests are green 😄
Is there still a task to be done before getting this PR merged?
Is there a plan to release a major version compatible with ES 7?

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Oct 14, 2019

@renanbr Main thing missing is me getting back from PTO and catching up on things. 7.0 release is planned yes. Hope to get all these in this and next week 🤞

@renanbr

This comment has been minimized.

Copy link

renanbr commented Oct 14, 2019

Thanks for the quick response! Let me know if you need some more hand 😉

@thePanz thePanz force-pushed the es-7-remove-type branch from 4644300 to b02a2c3 Oct 30, 2019
@thePanz

This comment has been minimized.

Copy link
Collaborator Author

thePanz commented Oct 30, 2019

@ruflin rebased and pushed!
I agree with having a 7.0.0-beta1 tag, that's would be great!

@thePanz thePanz self-assigned this Oct 30, 2019
@thePanz thePanz changed the title WIP: ES-7 remove type ES-7: remove type Oct 30, 2019
@ruflin ruflin merged commit 9979894 into master Oct 31, 2019
@ruflin ruflin deleted the es-7-remove-type branch Oct 31, 2019
@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Oct 31, 2019

Merged 🎉 🎈

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Oct 31, 2019

Just created #1685 to prepare the 7.0.0-beta1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.