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

Merge Prepared & Execute in execute.rs #167

Merged
merged 7 commits into from Mar 13, 2021
Merged

Merge Prepared & Execute in execute.rs #167

merged 7 commits into from Mar 13, 2021

Conversation

KyGost
Copy link
Contributor

@KyGost KyGost commented Mar 9, 2021

No description provided.

@panarch panarch self-requested a review March 9, 2021 13:40
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

I'll leave this comment, and you can check it tomorrow :)

It is not necessary to change behavior, Clone. because this should be a merging codes, it should not cause any behavioral changes.

 macro_rules! try_into {                          
     ($storage: expr, $expr: expr) => {           
         match $expr {                            
             Err(e) => {                          
                 return Err(($storage, e.into()));
             }                                    
             Ok(v) => ($storage, v),              
         }                                        
     };                                           
 }        

I saw that you used clone because of try_into!.

Here, more than one try_into! is used, so you need to change it slightly.

             Ok(v) => ($storage, v),              

Then, you can keep ownership of Store, you don't need to clone.

@KyGost
Copy link
Contributor Author

KyGost commented Mar 9, 2021

Hmm that seems like it might be a mess. I see now why they were separated.

@panarch panarch self-requested a review March 11, 2021 11:19
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

During the review, I got that what you might get stuck at.
Awesome, you solved all of that.

Let's make the codes more simple.
In this circumstance, keeping try_into! does not look like a good idea.

macro_rules! try_block {                     
    ($storage: expr, $block: block) => {{    
        match (|| async { $block })().await {
            Err(e) => {                      
                return Err(($storage, e));   
            }                                
            Ok(v) => v,                      
        }                                    
    }};                                      
}

Above one try_block! macro would be a good alternative.
You will be able to replace all try_into! to try_block!, let's remove try_into!.

In try_block! macro, we can use try operator ? without any issue.
No tricky closure and messy try_into! are required
I added a few examples of conversion. :)

src/tests/tester.rs Outdated Show resolved Hide resolved
src/executor/execute.rs Outdated Show resolved Hide resolved
src/executor/execute.rs Outdated Show resolved Hide resolved
src/executor/execute.rs Outdated Show resolved Hide resolved
@KyGost
Copy link
Contributor Author

KyGost commented Mar 11, 2021

Much nicer!

@KyGost
Copy link
Contributor Author

KyGost commented Mar 11, 2021

I left AlterTable because it would be a bit of a mess to use try_block there, try_into works nicer.

@KyGost KyGost mentioned this pull request Mar 12, 2021
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Beautiful :) Thanks so much!

I'd like to request only very tiny changes to remove #[allow(unused_macros)]

src/executor/execute.rs Outdated Show resolved Hide resolved
src/executor/execute.rs Show resolved Hide resolved
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Thanks! :) merging!

@panarch panarch merged commit 26f458c into gluesql:main Mar 13, 2021
@KyGost KyGost deleted the merge_prepared_execute branch March 13, 2021 02:32
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.

None yet

2 participants