Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refactor for SOLID compliance - Breaking on
getAllIndexes
(method return) andIndex
class (renamed intoIndexes
) #43Refactor for SOLID compliance - Breaking on
getAllIndexes
(method return) andIndex
class (renamed intoIndexes
) #43Changes from 24 commits
0e4c4bb
aa036c9
5475b55
57986d9
98ff789
c43ed53
4392ba3
9b12832
3674c77
4937034
68613bf
991c24a
231b1d4
9462e02
b1e1bcf
8fa5145
71ce5a1
598da68
1e2729d
228fe40
d4c723d
fca3abe
7e9da1e
8ca5694
9f98566
bcc5c12
3a6b7b7
b8658e5
227bf11
c876e12
9cf1ae3
ce28f33
fd092fa
504ae70
748337f
136b770
d4076c8
2a432b3
db9f02d
176a986
ee1c4dc
5e50cb1
1f5b92c
3892ef2
b9ceee8
9ac4495
071402d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return typehint for each method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that the responses can be
null
,array
, orstring
. So the return type has to bemixed
which I believe is same as not having a return type. Otherwise we will have to alter the response type be a DTO or something like your last comment. Which will break a lot of things in the public api. So I think the return types for these methods should be in a separate PR. with appropriate BC break labels or something, what do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, maybe it could be a good idea to submit a new PR that force more strict "logic" and types, but for now, that's okay for me 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this kind of big change would be appreciated, but in another PR 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be uppercase on
h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was thinking that the camelCased trait names would subtly indicate that its a trait instead of a class being used, If my memory is correct somewhere I have seen this practice (possibly laravel). So I had followed them in my projects as well.
But when I went back and checked they are also using CapitalCamelCase traits. So I have renamed all the traits to be CapitalCamelCased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this there no return value type on this method? I get why
deleteIndex
does not have a return type because MeiliSearch does not return anything, but should this one not bearray
too?And also, why is
handlesIndex
in this delegate folder./src/Delegates
and not./src/Delegates/Endpoints/Delegates
. What is the idea behind this architecture?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declaring the returned type in PHP is not mandatory, but it's a good practice since it has been introduced if I'm not wrong 🙂
I don't have any answer for the second point, I let @ppshobi doing it 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array
return type.handlesSearchSettings
tohandlesSettings
handlesIndex
trait is more like a proxy which transfers the calls to the actualIndexes
endpoint rather than the handling the actual http requests.All these methods were on theClient.php
directly, which was basically separated by comments only. So I thought of grouping them together and extracted out to a trait. Since it doesn't do any actual http calls, I am a bit hesitant putting it inside another directory named Endpoint. Also even if we do it, I believe it doesn't hurt as well.for example
previously
HttpRequest::class
class was extended byIndex::class
andClient::class
rather it can be likeIndex::class
can have a property ofHttpRequest::class
which is more clear. In my approach I have a dedicated class for making http requests and the endpoint classes have this http class which can be used to make meilisearch calls. Which I believe is more clear relationship.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be
: void
as return typehintThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uppercase on
h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return typehint for each method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uppercase on
h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return typehint on delete|update methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done