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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

General refactoring #76

Merged
merged 14 commits into from May 11, 2021
Merged

General refactoring #76

merged 14 commits into from May 11, 2021

Conversation

codedge
Copy link
Contributor

@codedge codedge commented Apr 27, 2021

This PR improves the code in various aspects

PHP version

Supported PHP versions are >=7.4 are >=8.0. Both, Symfony 4.4 and 5.0, can run using these versions.

Strict types

Using strict types is good. You know 馃槈

Type hints

By using PHP 7.4 as minimum version type hints can be set class properties. There are still places left in the code where multiple types are used or general object types. These place can (should?) be refactored at a later stage.

Comments

A lot of comments just describe the passed/parameter values and return values. Due to type hints they are obsolte.

Composer packages

The MeiliSearchImportCommand needs the Doctrine\Persistence\ManagerRegistry injected so Doctrine cannot only be a dev requirement.

I added the doctrine/orm package to the require section instead of require-dev.

This is also needed for the Symfony recipe PR I opened.

Tests

Some testing configuration seem to be unnecessary. I removed some configuration. Still, the tests run fine.

Holger Lo虉sken added 3 commits April 27, 2021 13:08
- Php version 7.4 or 8.0
- Use strict types
- Set type hints
- Remove unnecessary comments
- Remove not needed service injections for tests
@curquiza
Copy link
Member

curquiza commented Apr 27, 2021

@codedge thanks for your involvement and this refacto seems nice
Tell me when you think this is finished or if you want me to check something before continuing. I will check as soon as possible, but as you might have guessed with my abandoned PRs in this repo, I don't always have the time :) So your involvement on this repo is really helpful for me! Thanks again! 馃榿

If you are not already in our Slack, you can join us, it would be more convenient to discuss the PRs if needed: https://slack.meilisearch.com/

@codedge
Copy link
Contributor Author

codedge commented Apr 27, 2021

@curquiza Yep, saw your abandoned PRs and thought I could show some 鉂わ笍 to this repository.
I am going to fix the conflicts after we decided about the other open PRs that are left to be merged.

Joining Slack sound like a good plan. Already joined. 馃憤

@codedge codedge mentioned this pull request Apr 27, 2021
@codedge codedge mentioned this pull request Apr 30, 2021
3 tasks
@curquiza curquiza added the breaking-change The related changes are breaking for the users label May 10, 2021
@curquiza
Copy link
Member

curquiza commented May 10, 2021

Hello @codedge, sorry for the delay!!!

It looks good to me 馃檪
Can you just update the bors.toml file: removing the lines related to PHP 7.3 and 7.4 馃槆

Thanks again!

I've just gave you the write access to this repo: no need to work on your forked repo 馃檪

@codedge
Copy link
Contributor Author

codedge commented May 11, 2021

@curquiza Fixed the integration tests. After this PR has been merged, I'll continue to work on the repo itself, without the forked repo. Let's just close this one here :) ... and thanks for the access 馃憤

@codedge codedge requested a review from curquiza May 11, 2021 18:32
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again for your PR! And your help on this package 馃榿

bors merge

@bors
Copy link
Contributor

bors bot commented May 11, 2021

@bors bors bot merged commit 57825c3 into meilisearch:main May 11, 2021
@codedge codedge deleted the general-refactoring branch May 11, 2021 22:24
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.

None yet

2 participants