Skip to content

Conversation

@fogelito
Copy link
Contributor

No description provided.

@fogelito fogelito requested a review from abnegate April 10, 2023 10:49
Copy link
Member

@abnegate abnegate left a comment

Choose a reason for hiding this comment

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

Let's format the messages like the suggestions

fogelito and others added 5 commits April 10, 2023 15:07
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
@fogelito fogelito requested a review from abnegate April 10, 2023 12:39
public static function equal(string $attribute, array $values): self
{
if (empty($values)) {
throw new Exception('Equal queries require at least one value.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw error, or continue without a query?
As Appwrite developer, if I do [Query.equal('userId', ids]) and ids are empty, it would be annoying to get an error. It would mean I need to do exactly the same check in MANY places - everywhere, where I put array into query.

To keep SDK and HTTP consistent, I would avoid fixing this in SDK. I think this should be fixed here, or in Appwrite repo. I think empty array of values should return everything, instead of throwing error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not realize the SDK fixing.. We can do it with a validator if we want to throw an error
The question is Should we throw errors, or select all? especially now that we removed limits?
@abnegate what do you think

Copy link
Member

Choose a reason for hiding this comment

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

I think we should throw an error. In that way, the developer knows the query has an issue. They might not realize it's not doing anything if we just ignore it. Make more sense to do it in the validator though instead of here

@fogelito
Copy link
Contributor Author

Will be taken care of here...
#264

@fogelito fogelito closed this Apr 24, 2023
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.

4 participants