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 type to http client property #385

Closed
wants to merge 2 commits into from
Closed

Conversation

B3none
Copy link

@B3none B3none commented Oct 5, 2022

Pull Request

Related issue

Not related to an issue - should add contin

What does this PR do?

It adds the type to the private class property $http.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@curquiza
Copy link
Member

curquiza commented Oct 6, 2022

Hello @B3none

Thanks for your PR 😊
Since there is no issue, could you explain a little bit more about this change?
What is the purpose of it, I mean the value of changing this? If yes, is there any other part in the code base that would need this kind of change as well?

@B3none
Copy link
Author

B3none commented Oct 6, 2022

Hello @B3none

Thanks for your PR 😊 Since there is no issue, could you explain a little bit more about this change? What is the purpose of it, I mean the value of changing this? If yes, is there any other part in the code base that would need this kind of change as well?

@curquiza from what I’ve seen this is used everywhere, I think this one was missed because of the class name clash - so I’ve switched to an alias.

The change makes the code more declarative, improve the speed and reliability of typehint generation as well as not allow anyone to set the $http class property to anything other than what it’s expecting.

@curquiza
Copy link
Member

curquiza commented Oct 6, 2022

Ok thank you!
However, all the tests are failing, can you fix it so that I can merge the PR?

@B3none
Copy link
Author

B3none commented Oct 6, 2022

@curquiza try that - the linter is passing locally for me, struggling to run the integration tests

@B3none
Copy link
Author

B3none commented Oct 6, 2022

Yeah I give up with this, i've already got my 4 hacktoberfest PRs 😂

@B3none B3none closed this Oct 6, 2022
jonatanrdsantos added a commit to jonatanrdsantos/meilisearch-php that referenced this pull request Oct 13, 2022
bors bot added a commit that referenced this pull request Oct 13, 2022
392: Enforce strong type property - related with #385 r=brunoocasali a=jonatanrdsantos

# Pull Request

Add strongly typed property.

## Related with
Fixes #385

## What does this PR do?
- Applies suggestion of #385

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: jonatanrdsantos <[email protected]>
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