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

Use meilisearch prefix for services #266

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Jun 16, 2023

Pull Request

What does this PR do?

  • Improves "best practices/coding standard" as the « search » prefix is to generic, lets be specific for this bundle

This PR can be merged before #265 (as it will require a major release)

TODO:
Assert that the container has all services and that deprecations are triggered


RFC

I found some inconsistency reading the repo:

  • Commands namespace: meili:
  • Bundle config: meili_search
  • Services: search (this PR)

Will it be nice to define them as meilisearcheverywhere?

config/services.xml Outdated Show resolved Hide resolved
config/services.xml Outdated Show resolved Hide resolved
config/services.xml Outdated Show resolved Hide resolved
@94noni
Copy link
Contributor Author

94noni commented Jul 11, 2023

@norkunas thx for review, I will fix them ASAP
question: do you have a local project where you can test this PR? I coded but not run any project with it, just to check the depreciation on the profiler to be sure :)

@norkunas
Copy link
Collaborator

@norkunas thx for review, I will fix them ASAP question: do you have a local project where you can test this PR? I coded but not run any project with it, just to check the depreciation on the profiler to be sure :)

Currently unavailable to chec this in a project, but I'd suggest to add some tests https://symfony.com/doc/current/components/phpunit_bridge.html#write-assertions-about-deprecations to make sure that deprecations are triggered :)

config/services.xml Outdated Show resolved Hide resolved
config/services.xml Outdated Show resolved Hide resolved
config/services.xml Outdated Show resolved Hide resolved
@norkunas norkunas changed the title [WIP] Services naming Use meilisearch prefix for services Jul 13, 2023
@94noni
Copy link
Contributor Author

94noni commented Jul 13, 2023

I will need to rework yes as well as testing the PR code runtime locally it will be easier
But for now no much time so plz be patient

@94noni
Copy link
Contributor Author

94noni commented Jul 26, 2023

@norkunas thx really for your time checking this PR
I've checked this code on a testing project, container boot great and deprecations are here (tested with bin/console -vvv and some local code)

but I would greatly appreciate a local test as well from project maintainers

cheers

@brunoocasali
Copy link
Member

bors merge

@brunoocasali brunoocasali added the breaking-change The related changes are breaking for the users label Aug 3, 2023
meili-bors bot added a commit that referenced this pull request Aug 3, 2023
266: Use meilisearch prefix for services r=brunoocasali a=94noni

# Pull Request

## What does this PR do?
- Improves "best practices/coding standard" as the « search » prefix is to generic, lets be specific for this bundle

This PR can be merged before #265 (as it will require a major release)

TODO:
Assert that the container has all services and that deprecations are triggered 

- https://symfony.com/doc/5.4/bundles/best_practices.html#services
- https://symfony.com/doc/5.4/service_container/alias_private.html#deprecating-service-aliases



---------

# RFC

I found some inconsistency reading the repo:

- Commands namespace: `meili:`
- Bundle config: `meili_search`
- Services: `search` (this PR)

Will it be nice to define them as `meilisearch`everywhere? 


Co-authored-by: Antoine Makdessi <[email protected]>
@meili-bors
Copy link
Contributor

meili-bors bot commented Aug 3, 2023

Build failed:

@94noni
Copy link
Contributor Author

94noni commented Aug 3, 2023

i dont understand the fail here :s

@brunoocasali
Copy link
Member

bors merge

meili-bors bot added a commit that referenced this pull request Aug 3, 2023
266: Use meilisearch prefix for services r=brunoocasali a=94noni

# Pull Request

## What does this PR do?
- Improves "best practices/coding standard" as the « search » prefix is to generic, lets be specific for this bundle

This PR can be merged before #265 (as it will require a major release)

TODO:
Assert that the container has all services and that deprecations are triggered 

- https://symfony.com/doc/5.4/bundles/best_practices.html#services
- https://symfony.com/doc/5.4/service_container/alias_private.html#deprecating-service-aliases



---------

# RFC

I found some inconsistency reading the repo:

- Commands namespace: `meili:`
- Bundle config: `meili_search`
- Services: `search` (this PR)

Will it be nice to define them as `meilisearch`everywhere? 


Co-authored-by: Antoine Makdessi <[email protected]>
@meili-bors
Copy link
Contributor

meili-bors bot commented Aug 3, 2023

Build failed:

@brunoocasali
Copy link
Member

This seems to be related to something other than your code @94noni 😢

@norkunas norkunas removed the breaking-change The related changes are breaking for the users label Aug 8, 2023
@norkunas
Copy link
Collaborator

norkunas commented Aug 8, 2023

@brunoocasali removed breaking-change label as this does not break BC

@norkunas
Copy link
Collaborator

norkunas commented Aug 8, 2023

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Aug 8, 2023

@94noni 94noni closed this Aug 8, 2023
@meili-bors meili-bors bot merged commit 8151791 into meilisearch:main Aug 8, 2023
5 checks passed
@brunoocasali brunoocasali added the enhancement New feature or request label Sep 14, 2023
@94noni 94noni deleted the services-name branch June 27, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants