-
Notifications
You must be signed in to change notification settings - Fork 45
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
Various improvements to howto.md #180
base: master
Are you sure you want to change the base?
Conversation
Like last time, I made the following changes - Improvements to English grammar. I am a native English speaker, and have tried to use common phrasing idioms that are clear and concise. - Replaced example of `ozo::make_connection` with `conn_info[io]`, since `ozo::make_connection` is deprecated. - Added more explanations of how and why ozo does things the way it does, and tried to fill in details where I struggled initially when researching OZO. Please feel free to correct any mistakes I have made about the technical implementation details.
@@ -63,12 +69,12 @@ int main() { | |||
}; |
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.
If you will make new edits, can you remove unnecessary semicolon? It's confusing.
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.
Did I add that semi-colon...? The diff for the PR doesn't seem to indicate that was changed in this 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.
Of course, not. I think it is an improvement too. And I do not ask you to update your PR only for this change. But if other changes are required, it is good idea (not requirement) to apply this change.
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.
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.
Hi @jonesmz , thanks for the PR!
There's a lot of changes in my and your PRs and conflicts are inevident. For now, I revise the how-to section at all, so I guess it's better to merge your changes.
I'll review your PR in a few days. And then we can merge it to master.
Sorry for any inconvenience.
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.
@koshachy Ok, sounds good.
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.
Thanks for your PR.
I've reviewed the changes and have some questions.
Could you fix them?
Don't hesitate to ask questions if something is unclear or if you want to start a discussion.
You can omit minor issues (tagged Minor
).
@@ -1,18 +1,18 @@ | |||
# How to |
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.
In this case, we'll get the Table of Contents
chapter in the left navigation menu.
So I guess it's better to leave the How to
name here.
Here is a pic for a better understanding.
@@ -39,17 +39,23 @@ int main() { | |||
ozo::rows_of<std::int64_t, std::optional<std::string>> rows; | |||
|
|||
// Connection info with host and port to connect to | |||
auto conn_info = ozo::make_connection_info("host=... port=..."); | |||
// This also supports a PostgreSQL connection URI string | |||
ozo::connection_info<> conn_info = ozo::make_connection_info("host=... port=..."); |
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'm not quite sure about this change.
Could you explain the reason why did you change the auto
keyword to the specific type?
|
||
// For _SQL literal | ||
using namespace ozo::literals; | ||
|
||
std::int64_t myParam = ...; |
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.
It's better to provide valid code samples.
You can put all the additional information in the comment above the variable definition.
|
||
Notice that the second position of the tuple is `std::optional<std::string>`.This is because the `name` field of the table can be _NULL_. Empty optional represents a _NULL_ value (you can learn more about Nullable concept from the documentation). If if a _NULL_ value is retrieved from the database for a type that is not an `std::optional`, then a deserialization error will occur. | ||
Notice that the second position of the tuple is `std::optional<std::string>`.This is because the `name` field of the table can be _NULL_. An empty optional represents a _NULL_ value (you can learn more about Nullable concept from the OZO documentation). If if a _NULL_ value is retrieved from the database for a type that is not an `std::optional`, then a deserialization error will occur. |
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.
Minor: double space.
Now we need to create a connection information for database to connect to. This is our connection source which can create a connection for us as it will be needed (you can learn more about ConnectionSource and ConnectionProvider concepts from the documentation). It's also acceptable to provide a connection URI string, instead of the comma seperated version. | ||
Now we need to create a connection information for database to connect to. This is our connection source which can create a connection for us as it will be needed (you can learn more about `ConnectionSource` and `ConnectionProvider` concepts from the documentation). It's also acceptable to provide a connection URI string, instead of the comma seperated version. | ||
|
||
Here's how to construct a database query. |
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 don't think that "construct" is a proper word when we're talking about writing/making queries.
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.
Let's use make or write instead.
|
||
```cpp | ||
const auto query = "SELECT id, name FROM users_info WHERE amount>="_SQL + std::int64_t(25); | ||
std::int64_t myParam = ...; |
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.
Valid code samples.
See above.
You can also use `ozo::make_query()`, if you prefer to use parameterized queries with the `$1` style notation to indicate your query parameters: | ||
|
||
```cpp | ||
std::int64_t myParam = ...; |
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.
Valid code samples.
See above.
//... | ||
|
||
const auto query = "SELECT id, name FROM users_info WHERE amount>="_SQL + std::int64_t(25); | ||
std::int64_t myParam = ...; |
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.
Valid code samples.
See above.
|
||
Since the mapping is many **C++** types to one **PostgreSQL** type, you can always map another C++ type as PostgreSQL type. | ||
It defines a type mapping from the C++ type to the specified built-in PostgreSQL type. This mapping also adds compatibility for the various array representations supported by OZO. The current array represantations in OZO are std::vector, std::array, std::list, so for a type that you bind to `std::string`, you can use `std::vector<std::string>`, `std::array<std::string, N>`, `std::list<std::string>` in your queries. |
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 last sentence in this block is overloaded and it's really difficult to read it.
I guess it is enough to leave the second part of it, isn't it?
... by OZO. You can use
std::vector<std::string>
,std::array<std::string, N>
,std::list<std::string>
in your queries.
Or:
Since the current array representations in OZO are std::vector
, std::array
, std::list
, you can use them in your queries: std::vector<std::string>
, std::array<std::string, N>
, std::list<std::string>
.
|
||
E.g. we have an in-library `text` type definition like: | ||
This mapping is many-to-one, allowing you to map multiple C++ types to the same PostgreSQL type. For example, OZO has the built-in mapping for the `text` type like such: |
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.
Minor: You can omit all 'like such' and 'like so' expressions in this document that located before code blocks.
Use the colon sign instead.
Like last time, I made the following changes
ozo::make_connection
withconn_info[io]
, sinceozo::make_connection
is deprecated.Please feel free to correct any mistakes I have made about the technical implementation details.