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

Cleanup the code of the MySQL connector #509

Merged
merged 8 commits into from
Aug 5, 2023

Conversation

MeanSquaredError
Copy link
Contributor

@MeanSquaredError MeanSquaredError commented Aug 1, 2023

OK, I updated the PR based on our discussion and I think it is ready for a review.

I mentioned my concern regarding the renaming of the types ending in _t here
#500 (comment)
So it probably makes sense to postpone the merging of this PR until it is clear what to do with the renaming of these types.

This PR cleans up the code of the MySQL connector as per our discussion here
#500 (comment)

The PR makes the following changes to the MySQL connector:

  1. The code for the classes connection_handle, prepared_statement_handle and result_handle.h are moved to separate files in the
    include/sqlpp11/mysql/detail/ directory.
    2. All class and struct type names ending in _t have that suffix removed from their names. In some cases this caused some local variables and/or function parameters to be renamed in order avoid confusion and/or name clashes.
  2. sqlpp::mysql::connection_handle_t has been renamed to sqlpp::mysql::connection_handle as the other two connectors (PostgreSQL and SQLite3) have connection_handle without the _t suffix.
  3. sqlpp::mysql::serializer_t has been renamed to sqlpp::mysql::context_t
  4. Const qualifier has been added to sqlpp::mysql::context::escape() and its parameter has been changed from std::string to const std::string&.
    5. Old-style assignment-style parentheses-style initialization has been replaced with brace-style initialization. The only place where assignment-style initialization remains is auto var = {value} where C++11 has problems.
  5. There were several variables and one helper class using CamelCase. Those were renamed to snake_case as the project as a whole is using snake_case.

After making the changes the project was built with tests for the MySQL, PostgreSQL and SQLite3 connectors and all tests passed successfully.

@rbock
Copy link
Owner

rbock commented Aug 3, 2023

Thanks for experimenting with this. I think it makes sense to move code as suggested in the first three commits. I would prefer them as individual commits (if that makes sense, I have not checked them in detail).

The renaming requires more discussion, I think.

@MeanSquaredError MeanSquaredError force-pushed the cleanup_conn_mysql branch 2 times, most recently from b3fb143 to 0d46442 Compare August 3, 2023 10:15
@MeanSquaredError
Copy link
Contributor Author

@rbock
OK, I made changes to the PR and left the _t suffixes unchanged except for connection_handle. The other two connectors have connection_handle without the _t suffix, so I removed it from the MySQL connection handle too.

I think the PR is ready for a review, so please take a look.

@rbock
Copy link
Owner

rbock commented Aug 4, 2023

Looks good to me.

My only concern: This mixes several topics. In case something were broken with this CL, it would be easier to figure out the cause if they could be separate. The biggest change that is not related to moving code is the use of brace style initializers.

If this can be separated into its own PR easily, I would prefer that.

@MeanSquaredError
Copy link
Contributor Author

@rbock
OK, I removed the braced-init commit from the PR and tested it - all tests passed.
I will submit the braced-init changes in a separate PR.

Please review this PR and let me know if further changes are required.

@MeanSquaredError
Copy link
Contributor Author

Force-pushed an update that removed a braced init that sneaked into this PR from the other PR.

I think now this PR is ready for a review.

@rbock
Copy link
Owner

rbock commented Aug 5, 2023

Thanks for separating the changes! This looks good to me.

@rbock rbock merged commit 490259e into rbock:main Aug 5, 2023
@MeanSquaredError MeanSquaredError deleted the cleanup_conn_mysql branch August 5, 2023 11:20
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.

2 participants