Skip to content

Conversation

@Meldiron
Copy link
Contributor

@Meldiron Meldiron commented Sep 1, 2022

In Appwrite, we currently test queries by passing string versions into HTTP requests, such as

'queries' => [ 'limit(1)' ]

Thanks to introduction of toString(), we can now build actual query classes in Appwrite tests, such as

'queries' => [ Query::limit(1)->toString() ]

That will improve code quality and will make sure no unexpected test breaks occur when internal change is done to queries syntax.

*
* @return string
*/
public function toString(): string
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to implement __toString()?

Copy link
Member

Choose a reason for hiding this comment

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

@Meldiron I think this is a pretty cool suggestion, can we have the same thing for Role and Permission?

Copy link
Member

Choose a reason for hiding this comment

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

__toString() would only be used in the case where the object is contextually expected to be a string, e.g.

echo Query::limit(1);

For the case in the description, we would have to call the magic method:

'queries' => [ Query::limit(1)->__toString() ]

I think it's cleaner to implement an instance method toString() instead

@Meldiron Meldiron marked this pull request as draft October 3, 2022 07:07
@abnegate
Copy link
Member

@Meldiron What's left to move this out of draft state?

@Meldiron
Copy link
Contributor Author

This is not relevant anymore, closing the PR.
It was relevant as it was supposed to improve code and tests maintainability of Appwrite. Hit some hiccups that would require more refactoring. The benefits no longer outweighed the downsides.

@Meldiron Meldiron closed this Jan 10, 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.

5 participants