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

fang_tasks queries done #24

Merged
merged 10 commits into from
Jul 18, 2022
Merged

fang_tasks queries done #24

merged 10 commits into from
Jul 18, 2022

Conversation

pxp9
Copy link
Collaborator

@pxp9 pxp9 commented Jul 17, 2022

No description provided.

@pxp9 pxp9 requested a review from ayrat555 July 17, 2022 17:38
Copy link
Owner

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

tests please 👮

src/asynk/async_queue.rs Outdated Show resolved Hide resolved
.await
}
pub async fn remove_all_tasks(&mut self) -> Result<u64, AsyncQueueError> {
self.execute_one(REMOVE_ALL_TASK_QUERY, &[]).await
Copy link
Owner

Choose a reason for hiding this comment

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

this query may fail if the number of removed records is more than 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ayrat555 i am sure that query will not fail because execute_one calls

https://docs.rs/tokio-postgres/0.7.0/tokio_postgres/struct.Client.html#method.execute

that do not panic! if two or n records are deleted, execute method will return as u64 2 or n, that is then number of rows affected.

Copy link
Owner

Choose a reason for hiding this comment

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

@pxp9
Copy link
Collaborator Author

pxp9 commented Jul 18, 2022

Merge branch commit was in order to resolve a conflict.

Copy link
Owner

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

Great job 🚀

Added small comments

src/schema.rs Outdated Show resolved Hide resolved
assert_eq!(1, result);
let result = queue.remove_tasks_type("common").await.unwrap();
assert_eq!(2, result);
queue.transaction.unwrap().rollback().await.unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

I think you don't need to roll back. it will be rolled back automatically when queue is dropped

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ayrat555 It is true, you do not need to rollback explicitly but i think a rollback is an important operation that should be done explicitly, if you would to delete the explicit rollback, i will delete them.

Cargo.toml Outdated Show resolved Hide resolved
src/asynk/async_queue.rs Outdated Show resolved Hide resolved
@@ -76,13 +110,6 @@ where
return Err(AsyncQueueError::PoolAndTransactionEmpty);
};

if result != 1 {
Copy link
Owner

Choose a reason for hiding this comment

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

we can rename execute_one into execute and add additional parameter - expected_result_count: Option

if let Some(expected_result) = expected_result_count {
  if result != expected_result {
    return Err(AsyncQueueError::ResultError {
                expected: expected_result,
                found: result,
            });
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ayrat555 , Ok i will add this code, but we should call the function different because there is already an execute function Transaction::execute. Maybe will be better call it execute_test because we are gonna to pass an expected_result.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine to call it execute. because we're adding the method to our own struct

but you can call it checked_execute if you don't like just execute

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ayrat555 is a problem to add this parameter to expected_result_count: Option to all functions ?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine. but the code will be duplicated

Copy link
Collaborator Author

@pxp9 pxp9 Jul 18, 2022

Choose a reason for hiding this comment

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

@ayrat555 if we change all tests and only call Checked execute in the tests we can avoid the duplicated code.
what do you think ??

Copy link
Collaborator Author

@pxp9 pxp9 Jul 18, 2022

Choose a reason for hiding this comment

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

@ayrat555 why would you like this error in checked_execute ?
assert_eq! macro already gives the expected value and the value given.

image
I have modified the code temporarily to show this example.

Copy link
Owner

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

Fix clippy, and the pr is good to merge

@pxp9 pxp9 merged commit a60eb08 into ayrat555/async Jul 18, 2022
@ayrat555 ayrat555 deleted the async-sql-queries branch July 18, 2022 10:44
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