Skip to content

Conversation

@rmunate
Copy link
Contributor

@rmunate rmunate commented Sep 9, 2023

Hello engineers, I was taking a complete walk through the source code, I wanted to contribute with the adjustment of the comments and styles, this according to the same standards that the Laravel framework uses, I hope that although it is a very small contribution, it is useful and accepted, I hope then have time to be able to provide good and improved features.

@GautierDele
Copy link
Member

GautierDele commented Sep 10, 2023

Thanks @rmunate for the proposal it's a really good idea and work.

Since I added StyleCi on this repository, it made quite a lot of conflicts with your PR, so I did coauthored the rest of the PR on #39

About the try catch adding, can you give a bit of context on why you proposed this ? This is to force the rollback I think but i don't see why this is mandatory since the closed request won't commit ?

@rmunate
Copy link
Contributor Author

rmunate commented Sep 10, 2023

@GautierDele I took the time to adjust the conflicts, I hope with this you can consider the idea of combining, in response to your question I saw that you are using DB::beginTransaction() and DB::commit() to start and commit a transaction Database. This is excellent practice, however, it would also be good to handle possible database exceptions with DB::rollback() in case a transaction error occurs. Otherwise, pending transactions may be blocked. I think that any new contributor could intuit this in the same way, however if I am wrong I would apologize and delete that change.

@GautierDele
Copy link
Member

@rmunate
Copy link
Contributor Author

rmunate commented Sep 14, 2023

@GautierDele Hello engineer. Perfect, in that case I will close the request and I hope to be able to contribute to you in other things.

@rmunate rmunate closed this Sep 14, 2023
@GautierDele
Copy link
Member

I hope so too ! Thanks

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.

3 participants