Skip to content

Conversation

@curquiza
Copy link
Member

@curquiza curquiza commented Nov 13, 2020

⚠️ Should be merged after the merge of #175

Breaking change:

  • Make the internal method Index.create(...) a @classmethod instead of a @staticmethod. Why? Put the logic related to an index in the Index class instead of the Client class.
    See the difference between static and class methods in Python: https://stackabuse.com/pythons-classmethod-and-staticmethod-explained/.
  • Remove the internal method Index.get_indexes(). Since it's not related to only one index, this method should not be present in the Index class.
  • Make the update() method returns an Index object instead of a dict because it's more convenient to manipulate object instead of dict. This is consitent with client.index('movies').fetch_info() and client.create_index('movies) and client.index('movies')

Changes:

  • Check the type of the responses more accurately: assert isinstance(response, Index)

@curquiza curquiza added the breaking-change The related changes are breaking for the users label Nov 13, 2020
@curquiza curquiza changed the base branch from master to implicit-index-creation-v2 November 13, 2020 12:09
bidoubiwa
bidoubiwa previously approved these changes Nov 16, 2020
@curquiza curquiza requested a review from eskombro November 16, 2020 14:23
Copy link
Contributor

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

I really like it!

Just one suggestion

🌮

@curquiza curquiza force-pushed the restructure-code branch 2 times, most recently from 98ebb38 to 5414d3c Compare November 17, 2020 15:31
@curquiza curquiza force-pushed the implicit-index-creation-v2 branch from c37ee08 to ed86de1 Compare November 17, 2020 15:33
Base automatically changed from implicit-index-creation-v2 to master November 17, 2020 15:58
@bors bors bot dismissed bidoubiwa’s stale review November 17, 2020 15:58

The base branch was changed.

@curquiza curquiza closed this Nov 17, 2020
@curquiza curquiza reopened this Nov 17, 2020
@curquiza curquiza marked this pull request as ready for review November 17, 2020 16:32
Copy link
Contributor

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM !

@curquiza
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Nov 18, 2020

Build succeeded:

@bors bors bot merged commit d040e1f into master Nov 18, 2020
@bors bors bot deleted the restructure-code branch November 18, 2020 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change The related changes are breaking for the users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants