Skip to content

Conversation

SimonFrings
Copy link
Contributor

@SimonFrings SimonFrings commented Nov 23, 2023

In this pull request, I propose changing the current QueryResult to MysqlResult. This change aligns with the recently introduced MysqlClient from #186, ensuring a consistent "Mysql" prefix throughout.

The name "QueryResult" is not incorrect in this case. However, when working with multiple database clients in one project, it can lead to confusion about which specific "query result" is being referred to. For instance, the https://github.com/clue/reactphp-sqlite project has a similar class named Result, which should also be renamed to something like SqliteResult in the future.

Furthermore, we applied a similar reasoning in reactphp/socket#263 and reactphp/http#417.

Refs #147
Builds on top of #186

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

While I think React\MySQL\Result would be better than React\MySQL\MysqlResult it's not a deal breaker for me. And I do agree with the argument that it makes low-level usage of this package easier. And honestly it's going tobe abstracted away by an ORM anywa.

Copy link
Contributor

@clue clue left a comment

Choose a reason for hiding this comment

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

@SimonFrings Thanks for looking into this.

I agree that either name is debatable, but including the Mysql prefix definitely helps to avoid some name clashes, so let's go ahead with this! :shipit:

@clue clue merged commit adcf751 into friends-of-reactphp:0.7.x Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants