search

talaia-labs / rust-teos

The Eye of Satoshi - Lightning Watchtower
https://talaia.watch
MIT License
128 stars 62 forks source link

Add logging for data directory and config file paths #210

Closed anipaul2 closed 1 year ago

anipaul2 commented 1 year ago

Following up from #204.

fixes: https://github.com/talaia-labs/rust-teos/issues/202

sr-gi commented 1 year ago

The title and description of this PR need to be changed to something meaningful.

Explain why is it that you had to open this and closed 204 so we have context for future reference.

Mentioning that you run test and checked the code formatting is not necessary. CI does that so no PR can be merged without that being checked.

sr-gi commented 1 year ago

The same applies to 6dcb30b. "issue204" is not a descriptive commit message. Be mindful of code maintainability!

anipaul2 commented 1 year ago

The title and description of this PR need to be changed to something meaningful.

Explain why is it that you had to open this and closed 204 so we have context for future reference.

Mentioning that you run test and checked the code formatting is not necessary. CI does that so no PR can be merged without that being checked.

I have updated the PR title and description based on your feedback. Please let me know if any further changes are required or if you have any additional feedback. Thank you for your guidance!

anipaul2 commented 1 year ago

Last set of comments (hopefully).

Thank you for your valuable feedback. I have already implemented the final changes as suggested. I appreciate your guidance throughout this process. If there's anything else that needs to be adjusted, please let me know. I'm looking forward to having this PR merged and contributing further to the project.

sr-gi commented 1 year ago

Needs squashing and the resulting commit needs a proper title and description.

Check #211

anipaul2 commented 1 year ago

Needs squashing and the resulting commit needs a proper title and description.

Check #211

Needs squashing and the resulting commit needs a proper title and description.

Check #211

I have successfully squashed the commits and updated the commit message with a proper title and description as per your suggestions. Please have a look at the updated changes and let me know if there's anything else that needs to be done.

anipaul2 commented 1 year ago

Just a nit, LGTM otherwise

Done. Would you like me to squash the commits once more before we proceed?

sr-gi commented 1 year ago

Just a nit, LGTM otherwise

Done. Would you like me to squash the commits once more before we proceed?

Yep, make it a single commit

anipaul2 commented 1 year ago

Just a nit, LGTM otherwise

Done. Would you like me to squash the commits once more before we proceed?

Yep, make it a single commit

Done.

sr-gi commented 1 year ago

ACK a1f9bfa

anipaul2 commented 1 year ago

ACK a1f9bfa

Thank you for the review and approval! I appreciate your feedback.

anipaul2 commented 1 year ago

Hi @sr-gi, There was a small typo on line 246 which was written using HashMap. As, we are using vector now so I changed it to that. I merged the new commit and pushed again. Kindly review again.

mariocynicys commented 1 year ago

Please run cargo fmt.

anipaul2 commented 1 year ago

Please run cargo fmt.

Done. Added the suggested changes and checked with fmt and clippy.

anipaul2 commented 1 year ago

Some things were pending or reverted from previous reviews (check inline).

Also, reference the previous PR that was closed on the description of this one, like: Following up from , so this is not lost.

Sure, I'll include the reference of the linked PR too as you suggested.

sr-gi commented 1 year ago

Some things were pending or reverted from previous reviews (check inline). Also, reference the previous PR that was closed on the description of this one, like: Following up from , so this is not lost.

Sure, I'll include the reference of the linked PR too as you suggested.

You added the reference to the commit, not to this PR.

anipaul2 commented 1 year ago

Some things were pending or reverted from previous reviews (check inline). Also, reference the previous PR that was closed on the description of this one, like: Following up from , so this is not lost.

Sure, I'll include the reference of the linked PR too as you suggested.

You added the reference to the commit, not to this PR.

I have linked the PR that I closed on the description of the latest commit. I am not getting where should I add the reference to.

anipaul2 commented 1 year ago

Some things were pending or reverted from previous reviews (check inline). Also, reference the previous PR that was closed on the description of this one, like: Following up from , so this is not lost.

Sure, I'll include the reference of the linked PR too as you suggested.

You added the reference to the commit, not to this PR.

I removed it from the commit and updated the description. Link

anipaul2 commented 1 year ago

LGTM

Thanks for the quick review!

sr-gi commented 1 year ago

@mariocynicys would you mind giving this a look? We should try to adhere to the two-reviews rule when possible

sr-gi commented 1 year ago

This needs rebasing now

anipaul2 commented 1 year ago

This needs rebasing now

I did already. Am I missing something?

image
sr-gi commented 1 year ago

This needs rebasing now

I did already. Am I missing something?

image

You're at least one commit behind upstream master, pull from it and rebase

anipaul2 commented 1 year ago

Apologies for the inconvenience caused. I encountered some issues with the rebase and commits in the previous PR, so I decided to create a new one. Please review the changes, and let me know if there's anything else I need to address. Thank you for your understanding.

sr-gi commented 1 year ago

Please do not do this again. You need to get used to how to rebase branches without the need to close and create a new PR. This is an essential part of the OS development process and it may happen several times per PR. You cannot just close and open PR every time it happens.

anipaul2 commented 1 year ago

Please do not do this again. You need to get used to how to rebase branches without the need to close and create a new PR. This is an essential part of the OS development process and it may happen several times per PR. You cannot just close and open PR every time it happens.

I apologize for any inconvenience caused, and I appreciate your understanding and patience. Going forward, I will make sure to follow the best practices for Git and branch management, such as creating separate feature branches, rebasing properly, and resolving conflicts when necessary. The main wrong thing I did was to work in master branch. I got to learn a lot of new things because of this issue and how git works in depth.